Bug 1211366 - parentId validation in contextMenus. r=kmaglione

browser/components/extensions/ext-contextMenus.js

@@ -240,20 +240,33 @@ MenuItem.prototype = {
     return this._id;
   },
 
+  ensureValidParentId(parentId) {
+    if (parentId === undefined) {
+      return;
+    }
+    let menuMap = contextMenuMap.get(this.extension);
+    if (!menuMap.has(parentId)) {
+      throw new Error("Could not find any MenuItem with id: " + parentId);
+    }
+    for (let item = menuMap.get(parentId); item; item = item.parent) {
+      if (item === this) {
+        throw new Error("MenuItem cannot be an ancestor (or self) of its new parent.");
+      }
+    }
+  },
+
   set parentId(parentId) {
-    // TODO: cycle check here
+    this.ensureValidParentId(parentId);
+
     if (this.parent) {
       this.parent.detachChild(this);
     }
 
-    let menuMap = contextMenuMap.get(this.extension);
+    if (parentId === undefined) {
-    if (menuMap.has(parentId)) {
+      this.root.addChild(this);
+    } else {
+      let menuMap = contextMenuMap.get(this.extension);
       menuMap.get(parentId).addChild(this);
-    } else if (parentId !== undefined) {
-      // Unless the intention was to null the parentId, if we cannot find
-      // any item with the given id, we should throw.
-      let e = new Error("Could not find any MenuItem with id: " + parentId);
-      throw e;
     }
   },
 

browser/components/extensions/test/browser/browser_ext_contextMenus.js

@@ -54,7 +54,12 @@ add_task(function* () {
         { title: "child2", parentId: parentToDel, onclick: genericOnClick });
       browser.contextMenus.remove(parentToDel);
 
-      browser.test.notifyPass();
+      try {
+        browser.contextMenus.update(parent, { parentId: child2 });
+        browser.test.notifyFail();
+      } catch(e) {
+        browser.test.notifyPass();
+      }
     },
   });