1095717 - browser_newtab_update.js is failing when we run-by-dir on debug builds

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

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

In a continuing effort on bug 1057512, we are looking to run tests per directory instead of a a big large chunk.  This future proofs us for splitting into more chunks and it helps prevent tests from affecting other tests!

Here is a try run showing the failures:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=24074e9f7d38&searchQuery=mochitest-browser-chrome-1

10:31:50 INFO - 666 INFO checking window state
10:31:50 INFO - 667 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_update.js | A promise chain failed to handle a rejection: - at resource://app/modules/BrowserNewTabPreloader.jsm:304 - TypeError: this._browser.docShell is undefined
10:31:50 INFO - Stack trace:
10:31:50 INFO - HiddenBrowser.prototype._createBrowser/<@resource://app/modules/BrowserNewTabPreloader.jsm:304:7
10:31:50 INFO - Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:868:23
10:31:50 INFO - this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:747:7
10:31:52 INFO - ###!!! [Parent][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv, messages will be lost

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Joel, can you please try whether the two patches attached to bug 1077652 fix the above failures?

Comment 2

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

pushed: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=01600dc94c5d

Comment 3

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

that first push was subjected to server overload, I redid it later in the day:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=88a804df43cc&searchQuery=windows%20browser-chrome-1

It appears that those two patches are not helping :(

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Joel Maher (:jmaher) from comment #3)
> It appears that those two patches are not helping :(

Ok, I guess we should wait to see whether bug 1077652 as a whole (the new preload mechanism) can fix this. I am currently working on getting this landed asap.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0003-Bug-1095717-Bail-out-when-creating-the-next-preload-.patch

Looking at the failure, how about this? Can you please push this to try on top of your patches? I just had this happen locally when running a single newtab test.

Comment 6

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

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

Comment 7

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

Comment on attachment 8520599 [details] [diff] [review]
0003-Bug-1095717-Bail-out-when-creating-the-next-preload-.patch

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

this is green on try server:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=12005ab932bf

the fix looks straightforward, I would be excited to see this land.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8520599 [details] [diff] [review]
0003-Bug-1095717-Bail-out-when-creating-the-next-preload-.patch

The Promise returned by HostFrame.get() resolves on the next tick so the parent host frame might have been destroyed already at that point. When that is the case we seem to properly append the <browser> child but it doesn't get a frame loader + docShell anymore.

I could have added a wasDestroyed or similar flag to the HostFrame but checking whether there is a docShell seems more straightforward to me.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/6dafe00f178a

The cset says r=jaws instead of r=gavin. Should read my emails more closely, sorry :/

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6dafe00f178a