1407547 - FAIL | browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js | Uncaught exception

Status: RESOLVED FIXED

(Core :: Networking: JAR, defect, P2)

Description

[Gary Chen [:xeonchen]]

at chrome://mochitests/content/browser/browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js:98 - TypeError: otherTab is null Stack trace: dontTemporarilyShowAboutHome@chrome://mochitests/content/browser/browser

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc02cf06142c869f1f7a2ebe8929c2c62423d256&selectedJob=136147138

Comment 1

[Gary Chen [:xeonchen]]

Hi Mike, do you have any idea on this or do you know anyone to ask?

This bug happens when I enable bug 1373708, which makes some stuff off-main-thread during |nsJARChannel::AsyncOpen|.
I use |await|+|setTimeout| in a while-loop while |otherTab| is null, but it then goes timeout.
Looks like it's not a simple timing-issue.

Comment 2

[Mike de Boer [:mikedeboer]]

Hi Gary, I have no idea off-hand... can you reproduce it locally with you patches applied from bug 1373708?

If so, you should be able to see what's going on visually _and_ from Try runs you can check the screenshots.

Let's also see if Matheus is still around and can tell us why the addition of `await SessionSaver.run()` was necessary in bug 1306294? (I can open the TBPL logs anymore.)

Comment 3

[Matheus Longaray (:mlongaray)]

Hey Mike! Still around :) Sorry the delay, had to take a few minutes to catch up with the patch added in bug 1306294.

From what I can see we added `await SessionSaver.run()` to make sure our session gets saved prior to unloading a closed window. So when we access SessionStore calling undoCloseWindow(), our session data object is up to date (see at http://searchfox.org/mozilla-central/source/browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js#89).

I remember we were facing timing-related issues in bug 1306294 and that did the job for this test.

Comment 4

[Gary Chen [:xeonchen]]

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> Hi Gary, I have no idea off-hand... can you reproduce it locally with you
> patches applied from bug 1373708?
> 
> If so, you should be able to see what's going on visually _and_ from Try
> runs you can check the screenshots.
> 

Thank you! I need some time to check this.

Comment 5

[Gary Chen [:xeonchen]]

(In reply to Mike de Boer [:mikedeboer] from comment #2)
> Hi Gary, I have no idea off-hand... can you reproduce it locally with you
> patches applied from bug 1373708?
> 
> If so, you should be able to see what's going on visually _and_ from Try
> runs you can check the screenshots.
> 

I can reproduce this intermittently, so I don't have a chance to observe what happened when it failed.
It's has to be run with "--appname dist" and "--run-until-failure".

All I can see now is: in normal cases, [1] is called 4 times, but in the failure case it is called 3 times,
then we got a null |otherTab|.

Is there any possibility that a new tab is not completely created before the first time it tries to close the window?
Because I'm not sure if [2] is doing the check.

[1] https://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js#93
[2] https://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js#79

Comment 6

[Mike de Boer [:mikedeboer]]

(In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #5)
> I can reproduce this intermittently, so I don't have a chance to observe
> what happened when it failed.
> It's has to be run with "--appname dist" and "--run-until-failure".
> 
> All I can see now is: in normal cases, [1] is called 4 times, but in the
> failure case it is called 3 times,
> then we got a null |otherTab|.
> 
> Is there any possibility that a new tab is not completely created before the
> first time it tries to close the window?
> Because I'm not sure if [2] is doing the check.

This could indeed be the case!
So what I see is very wrong in that test is [1], where `switchTab` is used the wrong way. Or at least it's hacky and confusing.
This needs to change to:
```js
let tab = gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
```

I think that'd be more reliable.

[1] https://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/browser/base/content/test/urlbar/browser_urlbarAboutHomeLoading.js#79

Comment 7

[Gary Chen [:xeonchen]]

(In reply to Mike de Boer [:mikedeboer] from comment #6)
> (In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #5)
> > I can reproduce this intermittently, so I don't have a chance to observe
> > what happened when it failed.
> > It's has to be run with "--appname dist" and "--run-until-failure".
> > 
> > All I can see now is: in normal cases, [1] is called 4 times, but in the
> > failure case it is called 3 times,
> > then we got a null |otherTab|.
> > 
> > Is there any possibility that a new tab is not completely created before the
> > first time it tries to close the window?
> > Because I'm not sure if [2] is doing the check.
> 
> This could indeed be the case!
> So what I see is very wrong in that test is [1], where `switchTab` is used
> the wrong way. Or at least it's hacky and confusing.
> This needs to change to:
> ```js
> let tab = gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
> await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> ```
> 
> I think that'd be more reliable.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 1c4da216e00ac95b38a3f236e010b31cdfaae03b/browser/base/content/test/urlbar/
> browser_urlbarAboutHomeLoading.js#79

Unfortunately it's not the case...but the good news is I know why |win.gBrowser.selectedTab.previousSibling| is null.
That's because when it restores the previously closed window, occasionally the focused tab is the first tab instead of the second one.

Comment 8

[Gary Chen [:xeonchen]]

(In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #7)
> Unfortunately it's not the case...but the good news is I know why
> |win.gBrowser.selectedTab.previousSibling| is null.
> That's because when it restores the previously closed window, occasionally
> the focused tab is the first tab instead of the second one.

Actually it's only one tab restored in that case.

:Gijs I cann't figure out why it sometimes restores only one tab, do you have any idea?

Comment 9

[Gary Chen [:xeonchen]]

(In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #8)
> (In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #7)
> > Unfortunately it's not the case...but the good news is I know why
> > |win.gBrowser.selectedTab.previousSibling| is null.
> > That's because when it restores the previously closed window, occasionally
> > the focused tab is the first tab instead of the second one.
> 
> Actually it's only one tab restored in that case.
> 
> :Gijs I cann't figure out why it sometimes restores only one tab, do you
> have any idea?

I added a check to the test file and it fails.

[1] https://hg.mozilla.org/try/rev/1381214a459f22a47829a8d90c8691edafcd4be9#l1.39

Comment 10

[:Gijs (he/him)]

(In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #8)
> :Gijs I cann't figure out why it sometimes restores only one tab, do you
> have any idea?

   await TabStateFlusher.flush(win.gBrowser.selectedBrowser);
   await BrowserTestUtils.closeWindow(win);
   ok(SessionStore.getClosedWindowCount(), "Should have a closed window");
 
   await SessionSaver.run();
 
   windowOpenedPromise = BrowserTestUtils.waitForNewWindow();
   win = SessionStore.undoCloseWindow(0);
   await windowOpenedPromise;

Session restore won't add tabs synchronously when the window opens, and we're only waiting for the window to have opened (and, I think, delayed startup to have fired). We should presumably wait for session restore to have actually created the second tab in the new window. Probably something like this:

if (win.gBrowser.visibleTabs.length < 2) {
  await BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "TabOpen");
}

(I don't want to wait explicitly for the session restore event because it will be slower to occur, and at that point the second tab may have loaded already, which will then mean we'll time out waiting for the tabLoaded promise instead.)

(In reply to Mike de Boer [:mikedeboer] from comment #6)
> (In reply to Gary Chen [:xeonchen] (needinfo plz) from comment #5)
> > I can reproduce this intermittently, so I don't have a chance to observe
> > what happened when it failed.
> > It's has to be run with "--appname dist" and "--run-until-failure".
> > 
> > All I can see now is: in normal cases, [1] is called 4 times, but in the
> > failure case it is called 3 times,
> > then we got a null |otherTab|.
> > 
> > Is there any possibility that a new tab is not completely created before the
> > first time it tries to close the window?
> > Because I'm not sure if [2] is doing the check.
> 
> This could indeed be the case!
> So what I see is very wrong in that test is [1], where `switchTab` is used
> the wrong way. Or at least it's hacky and confusing.
> This needs to change to:
> ```js
> let tab = gBrowser.selectedTab = BrowserTestUtils.addTab(gBrowser);
> await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> ```
> 
> I think that'd be more reliable.

This doesn't work because we're trying to load about:newtab in this new tab, and browserLoaded() doesn't fire in that case if the (new tab) page comes from a preloaded browser/tab.

Comment 11

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1407547-wait-until-all-tabs-are-restored-r-miked.patch

(In reply to :Gijs (no reviews; PTO recovery mode) from comment #10)
>    await TabStateFlusher.flush(win.gBrowser.selectedBrowser);
>    await BrowserTestUtils.closeWindow(win);
>    ok(SessionStore.getClosedWindowCount(), "Should have a closed window");
>  
>    await SessionSaver.run();
>  
>    windowOpenedPromise = BrowserTestUtils.waitForNewWindow();
>    win = SessionStore.undoCloseWindow(0);
>    await windowOpenedPromise;
> 
> Session restore won't add tabs synchronously when the window opens, and
> we're only waiting for the window to have opened (and, I think, delayed
> startup to have fired). We should presumably wait for session restore to
> have actually created the second tab in the new window. Probably something
> like this:
> 
> if (win.gBrowser.visibleTabs.length < 2) {
>   await BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "TabOpen");
> }
> 
> (I don't want to wait explicitly for the session restore event because it
> will be slower to occur, and at that point the second tab may have loaded
> already, which will then mean we'll time out waiting for the tabLoaded
> promise instead.)

Thanks Gijs, it works!

Hi Mike, would you please help to review this simple patch?
Thank you.

Comment 12

[Gary Chen [:xeonchen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2e0bb869fe0e752b336f47e70267e96615eef5b

Comment 13

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1407547-wait-until-all-tabs-are-restored-r-miked.patch

make |waitForEvent| call later.

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8925192 [details] [diff] [review]
0001-Bug-1407547-wait-until-all-tabs-are-restored-r-miked.patch

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

Thanks! Looks good to me!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d5c2d176a45
Wait until all tabs are restored. r=mikedeboer

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/0d5c2d176a45