913780 - Permanent OS X mochitest-bc orange on UX: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877178_unregisterArea.js | Everything should be in its default state.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

As of https://tbpl.mozilla.org/?tree=UX&rev=53f83c3fc0aa:

Pushlog: https://hg.mozilla.org/projects/ux/pushloghtml?startID=392&endID=393

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877178_unregisterArea.js | Everything should be in its default state.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877178_unregisterArea.js | With a new toolbar and default placements, everything should still be in a default state.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877178_unregisterArea.js | When the toolbar is unregistered, everything should still be in a default state.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877447_skip_missing_ids.js | Should be in the default state.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877447_skip_missing_ids.js | Should be back in the default state, despite unknown button ID in placements.

Looks the same on all OS X versions, not happening anywhere besides OS X.

Note that there are other tests that verify that things are in the default state, and they succeeded. I don't know why these are failing, but not the other ones. Perhaps other tests (or these tests?) call reset() and that eliminates the cause of the issue here.

Matt, was there anything tricky about this merge?

Comment 1

[:Gijs (he/him)]

Casual look suggests this might be the new social mark button, which changes currentset (and, on UX, therefore changes placements). That new test should probably be resetting currentset once it's done. Shane, does that sound plausible?

Comment 2

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #1)
> Casual look suggests this might be the new social mark button, which changes
> currentset (and, on UX, therefore changes placements). That new test should
> probably be resetting currentset once it's done. Shane, does that sound
> plausible?

Testing locally at least confirms that, when run independently, the customizableUI tests are fine. When run after *only* browser/base/content/test/social tests, they are not. Marking the dependency.

Comment 3

[:Gijs (he/him)]

Hmm. Looks like it's *not* the newly introduced test in and of itself, but browser_social_chatwindow_resize.js. This causes us to overflow, which changes currentSet (which I'd argue is a bug on the CustomizableUI side). Somehow, after the resizing is done, post-bug-891219, we stay overflowed. I don't know why. This means inDefaultState is false once that test has run.

Reverting the two patches on UX, there are no, at the end of that particular test, we'd no longer be overflowing, and inDefaultState is true.

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> Reverting the two patches on UX, there are no, at the end of that particular
> test, we'd no longer be overflowing, and inDefaultState is true.

English is hard. I meant:

Reverting the two patches on UX, at the end of that particular test, we no longer overflow and inDefaultState is true (i.e., expected behaviour).

Comment 5

[:Gijs (he/him)]

I don't understand what's going on here. The overflow/resize events seem to happen in a similar timeframe, AFAICT, but somehow the widths reported are drastically different, and on current UX (ie, broken version) we end up with the splitter in the overflow panel, but not the webrtc, social-share-button, and social-toolbar-item:

[00:28:45.079] [node.id for (node of document.getElementById("nav-bar").customizationTarget.childNodes)].join(',')
[00:28:45.082] "urlbar-container,webrtc-status-button,social-share-button,social-toolbar-item"

This is particularly strange because the splitter is meant to come straight after the urlbar-container.

In any case, on broken UX, all the items in the overflow panel get a minsize of close to 1000, whereas when I back out the social patch, it's around 400 (meaning, when the test does its final resize back to 'normal' size, everything gets neatly put back).

(tracking, because I only just realized I totally forgot)

Comment 6

[Legacy TBPL/Treeherder Robot]

Gijs
https://tbpl.mozilla.org/php/getParsedLog.php?id=27554343&tree=UX
Rev5 MacOSX Mountain Lion 10.8 ux debug test mochitest-browser-chrome on 2013-09-08 06:27:31
revision: bcf1ac68fc5b
slave: talos-mtnlion-r5-048

TypeError: this.ports is null
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877178_unregisterArea.js | Everything should be in its default state.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877178_unregisterArea.js | With a new toolbar and default placements, everything should still be in a default state.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877178_unregisterArea.js | When the toolbar is unregistered, everything should still be in a default state.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877447_skip_missing_ids.js | Should be in the default state.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877447_skip_missing_ids.js | Should be back in the default state, despite unknown button ID in placements.
TypeError: tabItem.$favImage is null
TypeError: this.markup is undefined: InspectorPanel_onNavigatedAway/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:312
TypeError: e is undefined: callback@chrome://mochitests/content/browser/browser/devtools/shared/test/browser_observableobject.js:53

Comment 7

[:Gijs (he/him)]

So I dug into this in the weekend and this morning, and had a lot of help from Shane. Some things that may be useful:

1. To easily reproduce, just run ./mach mochitest-browser browser/base/content/test/social/browser_social_chatwindow_resize.js --keep-open
At the end of the test, the navbar is overflowing with just the urlbar showing. That shouldn't happen. (CustomizableUI.inDefaultState is false at this point... which should also not happen, but that's bug 913972)
This test is what causes the failures in our CustomizableUI tests later on.

2. Only backing out the new social marks button doesn't fix this. In other words, the test regression has to do with the removal of the old social marks button, which was inside the social-toolbar-item.

3. I *think* this is essentially to do with the overflow handler firing once the resizing code has restored the original window size. Before the removal of the old social marks button, it would fire when the window was still small. This causes a problem because previously, because the overflow fired for a small window, the minSize associated with the buttons that end up in the overflow panel was 'small' (around 4-500) and now it is 'big' (around 1008). This means that when the resize call fires at the end of the test, we don't restore any of the items, as the window isn't big enough. I don't know what's causing this difference. 

However, it is also strange that it fires when it does, and that the clientWidth and scrollWidth values (of the navbar) are different at this point (ie, whatever's firing the overflow event thinks there is still some kind of overflow). In particular, clientWidth is 1008, scrollWidth is 1012. AFAICT only the urlbar, urlbar-splitter, social-toolbar-item, and social-share-button are in the navbar at this point. The former takes 1008 pixels in width, and has no margin. The splitter takes 8 pixels but has 4 pixels of negative margin on either side. The social toolbar item and social share button are both hidden/empty (width 0), so shouldn't come into this. I am at a loss as to where the 4 pixels are meant to come from.

4. Considering all this, I also have no idea why this is only happening on OS X. I can reproduce on retina, but I don't know if it's retina-specific or OS X specific (in particular, I don't know what our test machines on TBPL are using)

5. Applying this patch to the test: https://pastebin.mozilla.org/3003440 fixes the issue in question, according to Shane. I don't understand why that'd help, either, although I suspect it might be to do with the provider being removed later than it is now (this would reduce the width of the social-toolbar-item).

I've logged a bunch of info and will attach that shortly. Jared, can you help out here? I'm reaching the end of my sanity in terms of figuring out what's causing this issue.

Comment 8

[:Gijs (he/him)]

Attachment: Log of broken case

Comment 9

[:Gijs (he/him)]

Attachment: working-log.txt

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Ok, I've got some time while I wait for some perf bisections to build, so I'm going to poke at this.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

This problem is reproducible on non-Retina displays.

Comment 12

[Jared Wein [:jaws] (please needinfo? me)]

My initial guess as to the cause of the issue (although it doesn't explain why it is OSX only) is based on how we are coalescing the resize events.

Comment 13

[Legacy TBPL/Treeherder Robot]

Gijs
https://tbpl.mozilla.org/php/getParsedLog.php?id=27586743&tree=UX
Rev5 MacOSX Mountain Lion 10.8 ux debug test mochitest-browser-chrome on 2013-09-09 09:13:34
revision: c44d006da6bd
slave: talos-mtnlion-r5-044

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877178_unregisterArea.js | Everything should be in its default state.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877178_unregisterArea.js | With a new toolbar and default placements, everything should still be in a default state.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877178_unregisterArea.js | When the toolbar is unregistered, everything should still be in a default state.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877447_skip_missing_ids.js | Should be in the default state.
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_877447_skip_missing_ids.js | Should be back in the default state, despite unknown button ID in placements.
TypeError: this._windows[aWindow.__SSi] is undefined
TypeError: this.markup is undefined: InspectorPanel_onNavigatedAway/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/inspector/inspector-panel.js:311
TypeError: e is undefined: callback@chrome://mochitests/content/browser/browser/devtools/shared/test/browser_observableobject.js:53

Comment 14

[:Gijs (he/him)]

Attachment: make overflowable toolbar care about search splitter,

So, the 4px discrepancy is the search splitter. We then overflow that. We should (a) not overflow it, and (b) we should make it update its location (or lack thereof) when we overflow items so that if the search bar (dis)appears, the splitter (dis)appears with it. This is enough to fix the test, but will not reinsert the splitter, which might break other things. This is because the ordering of elements changes because of the overflow, and so the conditions for inserting the splitter aren't met. We'll fix that in bug 913972.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 801747 [details] [diff] [review]
make overflowable toolbar care about search splitter,

Review of attachment 801747 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the removal of the Cu.reportError line.

Comment 16

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #15)
> Comment on attachment 801747 [details] [diff] [review]
> make overflowable toolbar care about search splitter,
> 
> Review of attachment 801747 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the removal of the Cu.reportError line.

Landed as: https://hg.mozilla.org/projects/ux/rev/4777f3127bba .

Comment 17

[:Gijs (he/him)]

This is gone.

Comment 18

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/4777f3127bba