Enable browser_thumbnails_bg_crash_during_capture.js on e10s

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

Trunk
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Summary: Fix browser_thumbnails_bg_crash_during_capture.js → Enable browser_thumbnails_bg_crash_during_capture.js on e10s
Comment hidden (mozreview-request)
(Assignee)

Comment 4

2 years ago
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

2 years ago
mozreview-review
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 :)
Attachment #8780293 - Flags: review?(markh) → review+
(Assignee)

Comment 6

2 years ago
Ah yeah, thanks!
(Assignee)

Comment 7

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6f2ba7787816
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.