1294509 - Enable browser_thumbnails_bg_crash_during_capture.js on e10s

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Comment 1

[Drew Willcoxon :adw]

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

Comment 2

[Drew Willcoxon :adw]

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

Comment 3

[Drew Willcoxon :adw]

Attachment: Bug 1294509 - Enable browser_thumbnails_bg_crash_during_capture.js on e10s.

Review commit: https://reviewboard.mozilla.org/r/70998/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70998/

Comment 4

[Drew Willcoxon :adw]

This patch delays recreating the browser when a crash occurs.  Seems to fix the problem.

This test queues up two captures.  While the first is ongoing, the test crashes the thumbnailing browser, so then the second should immediately start and finish properly.  But what happens is that after the browser is recreated for the second capture, something's wrong with its message manager.  When BackgroundPageThumbs sends the capture message to it, sendAsyncMessage throws with NS_ERROR_NOT_INITIALIZED.  That's where the test fails.  But it doesn't always happen, so the test is intermittent.  And this is only when e10s is enabled.

My guess is that it's not safe to try and respawn the content process in crash listeners because the OOP code just isn't ready to handle that yet or something.

Comment 5

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

Comment on attachment 8780293 [details]
Bug 1294509 - Enable browser_thumbnails_bg_crash_during_capture.js on e10s.

https://reviewboard.mozilla.org/r/70998/#review68582

Looks fine to me (with or without the suggestion below)

::: toolkit/components/thumbnails/BackgroundPageThumbs.jsm:239
(Diff revision 1)
> +        // immediately (on the same stack as the _done call stack) if there are
> +        // any more queued-up captures, and that seems to mess up the new
> +        // browser's message manager if it happens on the same stack as the
> +        // listener.  Trying to send a message to the manager in that case
> +        // throws NS_ERROR_NOT_INITIALIZED.
> +        let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);

I'd have thought Services.tm.currentThread.dispatch would be a little cleaner here and avoid the explicit strong reference? I don't really care though :)

Comment 6

[Drew Willcoxon :adw]

Ah yeah, thanks!

Comment 7

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/fx-team/rev/6f2ba7787816fd01f77c30e3bdb68400db74bf0c
Bug 1294509 - Enable browser_thumbnails_bg_crash_during_capture.js on e10s. r=markh

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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