1082130 - browser_967000_button_sync|tabview.js fails when running as a standalone directory

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

in bug 992911, we are looking to enable tests where we run a fresh browser instance per directory.  Usually what happens is that a few tests fail because they accidentally depend on the state of the browser from an earlier test.

In a try run:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6d8041d9592d

you can see that bc1 is orange across the board and for the most part it is the same tests failing on each platform:
09:35:02 INFO - 1230 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_967000_button_sync.js | A promise chain failed to handle a rejection: - at chrome://browser/content/aboutaccounts/aboutaccounts.js:299 - TypeError: window.location is null
09:35:02 INFO - 1234 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_967000_button_tabView.js | A promise chain failed to handle a rejection: - at chrome://browser/content/aboutaccounts/aboutaccounts.js:299 - TypeError: window.location is null

I am able to reproduce this locally while running:
./mach mochitest-browser browser/components/customizableui/test/browser_967000_button_sync.js

The errors here are sort of cryptic, is there a chance that this is an easy fix or you could give some guidance to me to fix this problem?

Comment 1

[:Gijs (out for now; he/him)]

This looks like an actual bug in sync's about:accounts not handling what happens if the tab has been closed by the time that the promise resolves. Alex, can you help move this forward?

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Note that bug 991057 fixed a similar problem, but it looks like that might have got lost in subsequent changes - just below the failing block we have:

    // tests in particular might cause the window to start closing before
    // getSignedInUser has returned.
    if (window.closed) {
      return;
    }

It looks like this just needs to be moved to the top of the block before we start introspecting window.location etc.

Comment 3

[Alex Bardas :alexbardas]

Attachment: patch v1

It's what Mark suggested. What I find strange is that I definitely ran only the tests for this file when I worked on the entrypoint bug.

Can it also be a fxAccount regression, taking longer than usual to respond?

Comment 4

[:Gijs (out for now; he/him)]

(In reply to Alex Bardas :alexbardas from comment #3)
> Created attachment 8504381 [details] [diff] [review]
> patch v1
> 
> It's what Mark suggested. What I find strange is that I definitely ran only
> the tests for this file when I worked on the entrypoint bug.
> 
> Can it also be a fxAccount regression, taking longer than usual to respond?

The tests that are failing aren't the tests for this file, they're the toolbar button tests which only landed very recently. They've just exposed this bug, that's all.

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8504381 [details] [diff] [review]
patch v1

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

Looks good, but please make sure it does indeed fix the tests mentioned by Gijs before landing (otherwise we might find there is something else we need to do...)

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

I verified it locally if that helps!

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Joel Maher (:jmaher) from comment #6)
> I verified it locally if that helps!

Ship it! :)

Comment 8

[Alex Bardas :alexbardas]

> Looks good, but please make sure it does indeed fix the tests mentioned by
> Gijs before landing (otherwise we might find there is something else we need
> to do...)

When I tried to run the tests only for ..._sync.js, 2 of them failed. No fails for ..._tabView.js though.

Also, when I locally run the tests for the whole directory: without this patch I get 2 failed tests, with it 0 failed tests.

I'll push to try run to see if it's fixing the problem.

Comment 9

[Alex Bardas :alexbardas]

Try run looks good:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=032a76b1755c

Comment 10

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/54b2b11d1895

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54b2b11d1895