Crash on startup with remote tabs and acceleration

RESOLVED WONTFIX

Status

()

Core
Graphics: Layers
--
critical
RESOLVED WONTFIX
4 years ago
4 years ago

People

(Reporter: milan, Assigned: dvander)

Tracking

({crash, stackwanted})

Trunk
x86
Mac OS X
crash, stackwanted
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [startupcrash][e10s])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Set these options:
user_pref("browser.tabs.remote", true);
user_pref("layers.offmainthreadcomposition.enabled", true);
user_pref("layers.offmainthreadcomposition.prefer-basic", true);
Run and crash.  The original report is for E10S branch, but this crashes on my OS X in the trunk.
(Reporter)

Comment 1

4 years ago
I get an assert in Layers.h:1814 at MOZ_ASSERT(aLayer->Manager() == Manager());  They do appear to be different.

Comment 2

4 years ago
(In reply to Milan Sreckovic [:milan] from comment #1)
> I get an assert in Layers.h:1814 at MOZ_ASSERT(aLayer->Manager() ==
> Manager());  They do appear to be different.

Hmm, I thought the whole point of RefLayers was to attach layers from a different LayerManager? (or at least a different layer tree and I thought that meant a different layer manager, but perhaps not). Does anyone actually understand ref layers?

I don't really know how browser.tabs.remote affects things actually.

Anyway, basic compositor is not really supported yet, so we probably don't need to worry too much about this for now. But it should be supported real soon, so we probably should worry about this a bit.

mattwoodrow is your man for basic layers OMTC.

Comment 3

4 years ago
Can you provide a crash ID?
Severity: normal → critical
Flags: needinfo?(milan)
Keywords: crash, stackwanted
Whiteboard: [startupcrash]
Version: unspecified → Trunk
(In reply to Nick Cameron [:nrc] from comment #2)
> (In reply to Milan Sreckovic [:milan] from comment #1)
> > I get an assert in Layers.h:1814 at MOZ_ASSERT(aLayer->Manager() ==
> > Manager());  They do appear to be different.
> 
> Hmm, I thought the whole point of RefLayers was to attach layers from a
> different LayerManager? (or at least a different layer tree and I thought
> that meant a different layer manager, but perhaps not). Does anyone actually
> understand ref layers?

I do :)

It builds a main layer tree (for the main process), plus a detached sub-tree for each child process. These all share the same LayerManagerComposite. At composite time we attach the sub-trees to the RefLayer attachment point in the main layer tree.

> 
> I don't really know how browser.tabs.remote affects things actually.
> 
> Anyway, basic compositor is not really supported yet, so we probably don't
> need to worry too much about this for now. But it should be supported real
> soon, so we probably should worry about this a bit.
> 
> mattwoodrow is your man for basic layers OMTC.

My guess is that the main process isn't getting OMTC, but the child process is trying to use it.

I can't reproduce this on a recent (within the last 12 hours) inbound build, it might have been fixed?
(Assignee)

Comment 5

4 years ago
I can reproduce this, taking.
Assignee: nobody → dvander
Blocks: 879538
Status: NEW → ASSIGNED
Flags: needinfo?(milan)
(Assignee)

Updated

4 years ago
Whiteboard: [startupcrash] → [startupcrash][e10s]
(Assignee)

Comment 6

4 years ago
Created attachment 769829 [details] [diff] [review]
fix

When setting browser.remote.tabs = true, tabbrowser.xml triggers nsBaseWidget::GetLayerManager to be called before the window is made opaque. We get a non-accelerated manager, but our child assumes accelerated layers, and sends a message that causes us to crash. 

This patch makes us forget our layer manager if our transparency changes.
Attachment #769829 - Flags: review?(roc)
Comment on attachment 769829 [details] [diff] [review]
fix

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

nice
Attachment #769829 - Flags: review?(roc) → review+
At some point we should think about making our LayerManager pointers a little less brittle. Bug 773097 was caused by us switching to an accelerated layer manager while in the middle of rendering using a BasicLayerManager; because the objects aren't refcounted, they just disappeared and the pointers we were using started pointing to bad memory.

Maybe the right solution to this is giving up on switching layer managers? I know they're a big startup win, but I fear we're going to keep having issues like this.
Flags: needinfo?(roc)
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(bas)
A big startup win is worth taking significant pain for. I don't think the pain here is that bad.
Flags: needinfo?(roc)
I'm not sure how big it is any more. We should measure.
This particular bug isn't related to our intentionally delayed creation of an accelerated layer manager on windows.

This bug happens because we happen to request a layer manager from the widget before all the properties that affect the layer manager type decision have been initialized.

The solution here is to reset the layer manager any time one of those properties is changed.

Not saying that we shouldn't revisit the delayed creation of a d3d9/10 layer manager, but this bug is not the place :)
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6b4a1ef09c
Backed out for mochitest failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/75f98ca9e995
(In reply to Joe Drew (:JOEDREW! \o/) from comment #8)
> At some point we should think about making our LayerManager pointers a
> little less brittle. Bug 773097 was caused by us switching to an accelerated
> layer manager while in the middle of rendering using a BasicLayerManager;
> because the objects aren't refcounted, they just disappeared and the
> pointers we were using started pointing to bad memory.
> 
> Maybe the right solution to this is giving up on switching layer managers? I
> know they're a big startup win, but I fear we're going to keep having issues
> like this.

I'll leave this up to Matt and Roc. I do think the delayed initialization stuff should be reconsidered, fwiw, it seems the startup-time obsession has decreased a little bit and that delay is a little bit of a pain. But I don't feel strongly about it.
Flags: needinfo?(bas)
I filed bug 890336 to evaluate it.
Flags: needinfo?(jmuizelaar)
Created attachment 777321 [details] [diff] [review]
Fixed fix

Failing mochitest error log: https://tbpl.mozilla.org/php/getParsedLog.php?id=24907242&tree=Mozilla-Inbound&full=1#error0

Looks like we need to also destroy the compositor when we get rid of the layer manager, added that. Passes the test locally for me.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=01201fe30350

Carrying forward r=roc.
Attachment #777321 - Flags: review+
Matt, is this ready to land?
Flags: needinfo?(matt.woodrow)
No, it regressed OSX 10.6. I couldn't reproduce it locally either unfortunately.
Flags: needinfo?(matt.woodrow)

Updated

4 years ago
Blocks: 895139
billm says WONTFIX.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
Duplicate of this bug: 895139
You need to log in before you can comment on or make changes to this bug.