The default bug view has changed. See this FAQ.

Figure out why browser_inspector_highlighter.js and browser_inspector_iframeTest.js fail when the style of the highlighter toolbar change

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: paul, Unassigned)

Tracking

Trunk
Firefox 11
Points:
---

Firefox Tracking Flags

(firefox10- fixed)

Details

(Whiteboard: [fixed-in-fx-team][qa-])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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]
(Reporter)

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
Same thing for browser_inspector_iframeTest.js. Adding an executeSoon fixed the problem.

So here, we are facing 2 race condition bugs.
(Reporter)

Comment 3

5 years ago
Ok, so I realize that even without the patch from bug 702776, these tests fail.
(Reporter)

Updated

5 years ago
No longer blocks: 702776
(Reporter)

Comment 4

5 years ago
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.
(Reporter)

Comment 5

5 years ago
Apparently, it is working for Win XP too.
(Reporter)

Comment 6

5 years ago
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?
(Reporter)

Comment 7

5 years ago
The transitions don't impact the results in the first and second test (tested with transitions disabled).
(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.
Whiteboard: [see comment 6]
Dave brought up that these machines are almost certainly running on VMs. Offering as another datapoint.

Comment 10

5 years ago
(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?
(Reporter)

Comment 11

5 years ago
(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.
(Reporter)

Comment 12

5 years ago
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
(Reporter)

Comment 13

5 years ago
Created attachment 575417 [details] [diff] [review]
patch v1
(Reporter)

Updated

5 years ago
Attachment #575417 - Flags: superreview?
Attachment #575417 - Flags: review?(rcampbell)
(Reporter)

Updated

5 years ago
Attachment #575417 - Flags: superreview?
(Reporter)

Comment 14

5 years ago
try-build running https://tbpl.mozilla.org/?tree=Try&rev=6cb4440d092e (with Dao's patch + this patch)
Whiteboard: [see comment 6]
(Reporter)

Comment 15

5 years ago
Apparently, this is working: https://tbpl.mozilla.org/?tree=Try&rev=6cb4440d092e
(Reporter)

Comment 16

5 years ago
Comment on attachment 575417 [details] [diff] [review]
patch v1

This patch is needed to fix bug 700333.
Attachment #575417 - Flags: approval-mozilla-aurora?
Comment on attachment 575417 [details] [diff] [review]
patch v1

testfix only, should be safe for aurora.
Attachment #575417 - Flags: review?(rcampbell) → review+
Status: NEW → ASSIGNED
Component: Developer Tools → Developer Tools: Inspector
QA Contact: developer.tools → developer.tools.inspector
(Reporter)

Updated

5 years ago
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/85f5dc707f6c
Assignee: paul → nobody
Status: ASSIGNED → NEW
Component: Developer Tools: Inspector → Developer Tools
QA Contact: developer.tools.inspector → developer.tools
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/85f5dc707f6c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
test-only fixes. Prevents test-failures from follow-up fixes.
tracking-firefox10: --- → ?

Updated

5 years ago
Attachment #575417 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/a3c26d14bbb2
status-firefox10: --- → fixed
Marking qa- as I don't think this is something QA needs to verify. Please mark qa+ if this is not the case.
Whiteboard: [fixed-in-fx-team] → [fixed-in-fx-team][qa-]

Updated

5 years ago
tracking-firefox10: ? → -
You need to log in before you can comment on or make changes to this bug.