Closed Bug 1026093 Opened 7 years ago Closed 6 years ago

Firefox crashes when loading Flash in an e10s window with hardware acceleration disabled

Categories

(Core :: Plug-ins, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
e10s m5+ ---
firefox33 --- affected
firefox35 --- affected
firefox38 --- fixed

People

(Reporter: cpeterson, Assigned: gw280)

References

(Blocks 1 open bug)

Details

(Keywords: crash, flashplayer, reproducible)

Crash Data

Attachments

(1 file, 2 obsolete files)

STR:
0. Create a new profile
1. Help > Restart with Add-ons Disabled
2. Start in Safe Mode
3. Open new e10s window
4. Load https://mail.mozilla.com/

RESULT 1:
Nothing happens! The page remains blank.

5. Load a page with Flash like youtube.com or cnn.com

RESULT 2:
CRASH! I see the following error messages on stderr:

###!!! [Child][MessageChannel::SendAndWait] Error: Channel error: cannot send/recv

[73698] ###!!! ABORT: Aborting on channel error.: file /builds/slave/m-cen-osx64-ntly-0000000000000/build/ipc/glue/MessageChannel.cpp, line 1532
[73698] ###!!! ABORT: Aborting on channel error.: file /builds/slave/m-cen-osx64-ntly-0000000000000/build/ipc/glue/MessageChannel.cpp, line 1532
See Also: → 1026521
Component: General → Plug-ins
Assignee: nobody → mrbkap
Priority: -- → P2
I can still reproduce this in Nightly 35:

bp-b4c32db9-86ab-40f6-ab5b-15f512140905
Crash Signature: [@ mozilla::layers::CompositorParent::ScheduleComposition() ]
Depends on: 1063848
Moving old M2 P2 bugs to M4.
I see this same behavior with hardware acceleration disabled, so it is a broader problem than just safe mode.  I think safe mode disables hardware acceleration.
Summary: Firefox crashes when loading Flash in an e10s window when running in Safe Mode ("Restart with Add-ons Disabled") → Firefox crashes when loading Flash in an e10s window with hardware acceleration disabled
If this problem happens when hardware acceleration is disabled outside of safe mode, then we should fix this sooner than M5.
Depends on: 1068199
We can workaround this crash with bug 1068199, so we can postpone this crash fix to M3 or later.
Blocks: 1068674
marking m4 since bug 1068674 is m4.
Renomming since bug 1068674 is no longer m4.
Hey Milan, how supported is hardware acceleration off on mac? In particular, we currently turn it off for "safe mode" (which currently turns off e10s as well, but we hope to change that in the future). We could potentially not turn off hardware acceleration in safe mode on Mac, but we shouldn't do that if we want to support the configuration in general.

What's the plan for that?
Flags: needinfo?(milan)
May as well just have one conversation in bug 1111859 - it certainly appears to be the same conversation.
Flags: needinfo?(milan)
See Also: → 1111859
Duplicate of this bug: 1111859
The plan here is to enable accelerated layers in the safe mode configuration for OS X only, based on the 99.9% success rate for OS X users running accelerated layers (http://people.mozilla.org/~bgirard/gfx_features_stats/#mac)
Assignee: mrbkap → gwright
Based on discussions with gfx, we think this is the correct way to go.
Attachment #8566706 - Flags: review?(mconley)
Comment on attachment 8566706 [details] [diff] [review]
0001-Bug-1026093-Don-t-allow-the-user-to-open-an-e10s-win.patch

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

::: browser/base/content/browser.js
@@ +7494,5 @@
>      let newNonRemoteWindow = document.getElementById("menu_newNonRemoteWindow");
>      let autostart = Services.appinfo.browserTabsRemoteAutostart;
> +
> +    if (!Services.appinfo.inSafeMode) {
> +      newRemoteWindow.hidden = autostart;

I'd actually prefer we return early before we do any of the work to get the DOM elements.

Like:

if (Services.appinfo.inSafeMode) {
  // Some comment about us not showing e10s menu items
  // in safe mode
  return;
}

let newRemoteWindow = ...
Attachment #8566706 - Flags: review?(mconley) → review-
Comment on attachment 8566706 [details] [diff] [review]
0001-Bug-1026093-Don-t-allow-the-user-to-open-an-e10s-win.patch

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

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1071,4 @@
>  // this button should never roll into production.
>  let buttonLabel = openRemote ? "New e10s Window"
>                                : "New Non-e10s Window";
> +let e10sDisabled = openRemote ? Services.appinfo.inSafeMode : false;

Also, this is kinda hard to parse. Why not just:

disabled: Services.appinfo.inSafeMode?
Attachment #8566706 - Attachment is obsolete: true
Attachment #8566737 - Flags: review?(mconley)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #14)
> Comment on attachment 8566706 [details] [diff] [review]
> 0001-Bug-1026093-Don-t-allow-the-user-to-open-an-e10s-win.patch
> 
> Review of attachment 8566706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/CustomizableWidgets.jsm
> @@ +1071,4 @@
> >  // this button should never roll into production.
> >  let buttonLabel = openRemote ? "New e10s Window"
> >                                : "New Non-e10s Window";
> > +let e10sDisabled = openRemote ? Services.appinfo.inSafeMode : false;
> 
> Also, this is kinda hard to parse. Why not just:
> 
> disabled: Services.appinfo.inSafeMode?

we only want to disable it if it's to open a remote window, right? This same element is used for both "open e10s" and "open non-e10s" window.
(In reply to George Wright (:gw280) from comment #16)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #14)
> > Comment on attachment 8566706 [details] [diff] [review]
> > 0001-Bug-1026093-Don-t-allow-the-user-to-open-an-e10s-win.patch
> > 
> > Review of attachment 8566706 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: browser/components/customizableui/CustomizableWidgets.jsm
> > @@ +1071,4 @@
> > >  // this button should never roll into production.
> > >  let buttonLabel = openRemote ? "New e10s Window"
> > >                                : "New Non-e10s Window";
> > > +let e10sDisabled = openRemote ? Services.appinfo.inSafeMode : false;
> > 
> > Also, this is kinda hard to parse. Why not just:
> > 
> > disabled: Services.appinfo.inSafeMode?
> 
> we only want to disable it if it's to open a remote window, right? This same
> element is used for both "open e10s" and "open non-e10s" window.

True, but if we're in safe mode, how can we get into a state where the button says "open non-e10s window"?
Flags: needinfo?(gwright)
That's.. a fair point.
Flags: needinfo?(gwright)
Attachment #8566737 - Attachment is obsolete: true
Attachment #8566737 - Flags: review?(mconley)
Attachment #8566767 - Flags: review?(mconley)
Comment on attachment 8566767 [details] [diff] [review]
0001-Bug-1026093-Don-t-allow-the-user-to-open-an-e10s-win.patch

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

By inspection, this LGTM. Thanks, George.
Attachment #8566767 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/d64f901863d6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Is this a potential data loss issue for users with remembered session?
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.