Closed Bug 1407547 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: xeonchen, Assigned: xeonchen)

References

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file, 1 obsolete file)

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
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.
Flags: needinfo?(mdeboer)
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.)
Flags: needinfo?(mdeboer) → needinfo?(mlongaray)
Flags: needinfo?(xeonchen)
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.
Flags: needinfo?(mlongaray)
(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.
(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
Flags: needinfo?(xeonchen) → needinfo?(mdeboer)
(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
Flags: needinfo?(mdeboer)
(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.
(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?
Flags: needinfo?(gijskruitbosch+bugs)
(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
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Attachment #8924757 - Flags: review?(mdeboer)
make |waitForEvent| call later.
Attachment #8924757 - Attachment is obsolete: true
Attachment #8924757 - Flags: review?(mdeboer)
Attachment #8925192 - Flags: review?(mdeboer)
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!
Attachment #8925192 - Flags: review?(mdeboer) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d5c2d176a45
Wait until all tabs are restored. r=mikedeboer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d5c2d176a45
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.