Bug 1284125 - Fix intermittent browser_inspector_delete-selected-node-02.js. r=jdescottes, a=test-only
authorSami Jaktholm <sjakthol@outlook.com>
Sat, 13 Aug 2016 10:14:01 +0300
changeset 350220 f35ba1281407025c188ab7f1aa0cf56f0d5e940c
parent 350219 2dadc5e11d6298faa2e7b11cdf8ca37f0187cc80
child 350221 2d86f107aea835f0b56176871a43d144c7fff78c
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdescottes, test-only
bugs1284125
milestone50.0a2
Bug 1284125 - Fix intermittent browser_inspector_delete-selected-node-02.js. r=jdescottes, a=test-only The intermittency is caused by a race condition. When delete is clicked in the context menu, markupview changes the selection and sends the removeNode request to the server. The selection change triggers the usual inspector update process in the client. At the same time, the server removes the node and queues the mutation triggered by the removal. And here lies the issue. If the inspector components finish updating BEFORE the removal mutation is received from the server, the test continues before the breadcrumbs learn about the deletion and the test fails. If the update is still pending when the mutation is received, the breadcrumbs have time to update before the test continues to make assertions about the breadcrumb contents. The fix here is to make the test to ensure that the breadcrumbs have seen the deletion before it continues. To enable that, the breadcrumbs need to tell the world that it has been updated even if the breadcrumbs were just trimmed and a non-element node was selected that does not trigger the full update process (early return in the code). As the inspector update process has been collecting cruft for years and tests make a lot of assumptions about the emitted events, it's not safe to trigger a new inspector update in this special case. Therefore, only the breadcrumbs-updated event is emitted in this special case and the test waits for that if the deleted node is still present in the breadcrumbs. MozReview-Commit-ID: AjC6k6SzLCu
devtools/client/inspector/breadcrumbs.js
devtools/client/inspector/test/browser_inspector_delete-selected-node-02.js
--- a/devtools/client/inspector/breadcrumbs.js
+++ b/devtools/client/inspector/breadcrumbs.js
@@ -828,34 +828,40 @@ HTMLBreadcrumbs.prototype = {
 
     if (!this.selection.isConnected()) {
       // remove all the crumbs
       this.cutAfter(-1);
       return;
     }
 
     // If this was an interesting deletion; then trim the breadcrumb trail
+    let trimmed = false;
     if (reason === "markupmutation") {
       for (let {type, removed} of mutations) {
         if (type !== "childList") {
           continue;
         }
 
         for (let node of removed) {
           let removedIndex = this.indexOf(node);
           if (removedIndex > -1) {
             this.cutAfter(removedIndex - 1);
+            trimmed = true;
           }
         }
       }
     }
 
     if (!this.selection.isElementNode()) {
       // no selection
       this.setCursor(-1);
+      if (trimmed) {
+        // Since something changed, notify the interested parties.
+        this.inspector.emit("breadcrumbs-updated", this.selection.nodeFront);
+      }
       return;
     }
 
     let idx = this.indexOf(this.selection.nodeFront);
 
     // Is the node already displayed in the breadcrumbs?
     // (and there are no mutations that need re-display of the crumbs)
     if (idx > -1 && !hasInterestingMutations) {
--- a/devtools/client/inspector/test/browser_inspector_delete-selected-node-02.js
+++ b/devtools/client/inspector/test/browser_inspector_delete-selected-node-02.js
@@ -88,16 +88,17 @@ add_task(function* () {
 
     expectedCrumbs = ["html", "body", "div#deleteToMakeSingleTextNode"];
     yield assertNodeSelectedAndCrumbsUpdated(expectedCrumbs,
                                              Node.ELEMENT_NODE);
   }
 
   function* deleteNodeWithContextMenu(selector) {
     yield selectNode(selector, inspector);
+    let nodeToBeDeleted = inspector.selection.nodeFront;
 
     info("Getting the node container in the markup view.");
     let container = yield getContainerForSelector(selector, inspector);
 
     let allMenuItems = openContextMenuAndGetAllItems(inspector, {
       target: container.tagLine,
     });
     let menuItem = allMenuItems.find(item => item.id === "node-menu-delete");
@@ -106,16 +107,26 @@ add_task(function* () {
     is(menuItem.disabled, false, "delete menu item is enabled");
     menuItem.click();
 
     // close the open context menu
     EventUtils.synthesizeKey("VK_ESCAPE", {});
 
     info("Waiting for inspector to update.");
     yield inspector.once("inspector-updated");
+
+    // Since the mutations are sent asynchronously from the server, the
+    // inspector-updated event triggered by the deletion might happen before
+    // the mutation is received and the element is removed from the
+    // breadcrumbs. See bug 1284125.
+    if (inspector.breadcrumbs.indexOf(nodeToBeDeleted) > -1) {
+      info("Crumbs haven't seen deletion. Waiting for breadcrumbs-updated.");
+      yield inspector.once("breadcrumbs-updated");
+    }
+
     return menuItem;
   }
 
   function* assertNodeSelectedAndCrumbsUpdated(expectedCrumbs,
                                                expectedNodeType) {
     info("Performing checks");
     let actualNodeType = inspector.selection.nodeFront.nodeType;
     is(actualNodeType, expectedNodeType, "The node has the right type");