Last Comment Bug 703237 - Figure out why browser_inspector_highlighter.js and browser_inspector_iframeTest.js fail when the style of the highlighter toolbar change
: Figure out why browser_inspector_highlighter.js and browser_inspector_iframeT...
Status: RESOLVED FIXED
[fixed-in-fx-team][qa-]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 11
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 700333
  Show dependency treegraph
 
Reported: 2011-11-17 03:35 PST by Paul Rouget [:paul]
Modified: 2012-01-05 13:39 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
fixed


Attachments
fix (1.29 KB, patch)
2011-11-17 04:32 PST, Paul Rouget [:paul]
no flags Details | Diff | Splinter Review
patch v1 (2.79 KB, patch)
2011-11-18 03:17 PST, Paul Rouget [:paul]
rcampbell: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Paul Rouget [:paul] 2011-11-17 03:35:47 PST
Happens with bug 700333 (WinXP only) and bug 702776 (Linux only)

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_highlighter.js | transparent veil box width matches dimensions of element (no zoom) - Got 0, expected 22
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_highlighter.js | transparent veil box width matches width of element (2x zoom) - Got 937, expected 44
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_highlighter.js | transparent veil box height matches width of element (2x zoom) - Got 76, expected 44
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_iframeTest.js | selection matches div2 node - Got [object HTMLDivElement], expected [object HTMLDivElement]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/highlighter/test/browser_inspector_iframeTest.js | highlighter matches selection - Got [object HTMLDivElement], expected [object HTMLDivElement]
Comment 1 Paul Rouget [:paul] 2011-11-17 03:51:02 PST
The tests in browser_inspector_highlighter.js don't take into account the fact that the veil is animated with a transition, so it "takes time" before reaching its final size. Adding a executeSoon or a setTimeout(..., 100) fixes the first problem.
Comment 2 Paul Rouget [:paul] 2011-11-17 04:07:37 PST
Same thing for browser_inspector_iframeTest.js. Adding an executeSoon fixed the problem.

So here, we are facing 2 race condition bugs.
Comment 3 Paul Rouget [:paul] 2011-11-17 04:20:50 PST
Ok, so I realize that even without the patch from bug 702776, these tests fail.
Comment 4 Paul Rouget [:paul] 2011-11-17 04:32:35 PST
Created attachment 575145 [details] [diff] [review]
fix

Apparently, this fixes the problem under Linux. Let's see if it works WinXP (see https://tbpl.mozilla.org/?tree=Try&rev=560a277e1926 Can't reproduce on my WinXP build).

Also, I am not sure to understand why Dao's patch "switch" the events order in the test. Can the transition be a bit "slower" with Dao's patch?

If this patch fixes the problem, I'll investigate this.
Comment 5 Paul Rouget [:paul] 2011-11-17 06:59:04 PST
Apparently, it is working for Win XP too.
Comment 6 Paul Rouget [:paul] 2011-11-17 07:14:25 PST
So I have a fix, but I can't explain why it works:
Let's only talk about browser_inspector_iframeTest.js for now.

If I wrap this part:

    is(InspectorUI.selection, div2, "selection matches div2 node");
    is(InspectorUI.highlighter.highlitNode, div2, "highlighter matches selection");

    finish();

into a `executeSoon()`, it works (if not, it fails with the above errors, and `InspectorUI.selection` is `div1`).

So it would mean that `performTestComparisons2` is call before `InspectorUI.selection` is updated.
So it would mean that `InspectorUI.INSPECTOR_NOTIFICATIONS.HIGHLIGHTING` is fired before `InspectorUI.selection` is set.

But this is not the case: In `IUI_select`,
      this.selection = aNode;
      if (!this.inspecting) {
        this.highlighter.highlightNode(this.selection);
      }
(`InspectorUI.INSPECTOR_NOTIFICATIONS.HIGHLIGHTING` is fired in `highlightNode`).

Help?
Comment 7 Paul Rouget [:paul] 2011-11-17 09:02:30 PST
The transitions don't impact the results in the first and second test (tested with transitions disabled).
Comment 8 Rob Campbell [:rc] (:robcee) 2011-11-17 15:30:54 PST
(In reply to Paul Rouget [:paul] from comment #6)
> So it would mean that `performTestComparisons2` is call before
> `InspectorUI.selection` is updated.
> So it would mean that `InspectorUI.INSPECTOR_NOTIFICATIONS.HIGHLIGHTING` is
> fired before `InspectorUI.selection` is set.
> 
> But this is not the case: In `IUI_select`,
>       this.selection = aNode;
>       if (!this.inspecting) {
>         this.highlighter.highlightNode(this.selection);
>       }
> (`InspectorUI.INSPECTOR_NOTIFICATIONS.HIGHLIGHTING` is fired in
> `highlightNode`).
> 
> Help?

so, that shouldn't happen. I wonder if there's some JIT optimization that's causing this to fire out of order? Otherwise I have no idea why this could happen.
Comment 9 Rob Campbell [:rc] (:robcee) 2011-11-17 15:32:48 PST
Dave brought up that these machines are almost certainly running on VMs. Offering as another datapoint.
Comment 10 Dave Camp (:dcamp) 2011-11-17 15:38:17 PST
(In reply to Paul Rouget [:paul] from comment #6)

> So it would mean that `performTestComparisons2` is call before
> `InspectorUI.selection` is updated.
> So it would mean that `InspectorUI.INSPECTOR_NOTIFICATIONS.HIGHLIGHTING` is
> fired before `InspectorUI.selection` is set.
> 
> But this is not the case: In `IUI_select`,
>       this.selection = aNode;
>       if (!this.inspecting) {
>         this.highlighter.highlightNode(this.selection);
>       }
> (`InspectorUI.INSPECTOR_NOTIFICATIONS.HIGHLIGHTING` is fired in
> `highlightNode`).

Are we getting an unexpected element being highlighted before the one we expect?
Comment 11 Paul Rouget [:paul] 2011-11-17 16:53:16 PST
(In reply to Dave Camp (:dcamp) from comment #10)
> Are we getting an unexpected element being highlighted before the one we
> expect?

Yes. Div1 is the selected node in these tests, not Div2. Actually, the problem is that InspectorUI.INSPECTOR_NOTIFICATIONS.HIGHLIGHTING is fired more often than expected. For example, even if the last `inspectNode` is commented, `finishTestComparisons` is still called.

I don't know why yet. I can reproduce this now on my machine, it's going to be much easier to debug.
Comment 12 Paul Rouget [:paul] 2011-11-18 03:16:44 PST
Found the problem: we add an observer listening to the HIGHLIGHTING message in a function that is, itself, an observer called for the HIGHLIGHTING message.

We are doing something like that:

addObserver(function obs1() {           // For div1
  // Remove obs1

  addObserver(function obs2() {         // For div2
     // Do something.
  }, HIGHLIGHTING);

  // executeSoon(Highlight div2)

}, HIGHLIGHTING);

We see "// Do something" being called for 'div1'.

So an observer is added while the list of observer for the same message are being called, appended at the end of the list of the observers that are been unrolled at this exact same time.

Moving the addObserver in executeSoon fixes the problem.

#yodawg
Comment 13 Paul Rouget [:paul] 2011-11-18 03:17:16 PST
Created attachment 575417 [details] [diff] [review]
patch v1
Comment 14 Paul Rouget [:paul] 2011-11-18 04:48:12 PST
try-build running https://tbpl.mozilla.org/?tree=Try&rev=6cb4440d092e (with Dao's patch + this patch)
Comment 15 Paul Rouget [:paul] 2011-11-18 07:07:10 PST
Apparently, this is working: https://tbpl.mozilla.org/?tree=Try&rev=6cb4440d092e
Comment 16 Paul Rouget [:paul] 2011-11-18 07:58:22 PST
Comment on attachment 575417 [details] [diff] [review]
patch v1

This patch is needed to fix bug 700333.
Comment 17 Rob Campbell [:rc] (:robcee) 2011-11-18 11:33:59 PST
Comment on attachment 575417 [details] [diff] [review]
patch v1

testfix only, should be safe for aurora.
Comment 18 Rob Campbell [:rc] (:robcee) 2011-11-19 08:54:30 PST
https://hg.mozilla.org/integration/fx-team/rev/85f5dc707f6c
Comment 19 Rob Campbell [:rc] (:robcee) 2011-11-21 06:45:05 PST
https://hg.mozilla.org/mozilla-central/rev/85f5dc707f6c
Comment 20 Rob Campbell [:rc] (:robcee) 2011-11-24 08:30:07 PST
test-only fixes. Prevents test-failures from follow-up fixes.
Comment 21 Rob Campbell [:rc] (:robcee) 2011-11-29 08:38:26 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3c26d14bbb2
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-28 13:47:28 PST
Marking qa- as I don't think this is something QA needs to verify. Please mark qa+ if this is not the case.

Note You need to log in before you can comment on or make changes to this bug.