886988 - Crash on startup with remote tabs and acceleration

Status: RESOLVED WONTFIX

(Core :: Graphics: Layers, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 2

[Nick Cameron [:nrc]]

(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

[Scoobidiver (away)]

Can you provide a crash ID?

Comment 4

[Matt Woodrow (:mattwoodrow)]

(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?

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

I can reproduce this, taking.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: 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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 769829 [details] [diff] [review]
fix

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

nice

Comment 8

[Joe Drew (not getting mail)]

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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

A big startup win is worth taking significant pain for. I don't think the pain here is that bad.

Comment 10

[Joe Drew (not getting mail)]

I'm not sure how big it is any more. We should measure.

Comment 11

[Matt Woodrow (:mattwoodrow)]

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 :)

Comment 12

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6b4a1ef09c

Comment 13

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

Backed out for mochitest failures: https://hg.mozilla.org/integration/mozilla-inbound/rev/75f98ca9e995

Comment 14

[Bas Schouten (:bas.schouten)]

(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.

Comment 15

[Joe Drew (not getting mail)]

I filed bug 890336 to evaluate it.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: 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.

Comment 17

[David Zbarsky (:dzbarsky)]

Matt, is this ready to land?

Comment 18

[Matt Woodrow (:mattwoodrow)]

No, it regressed OSX 10.6. I couldn't reproduce it locally either unfortunately.

Comment 19

[Chris Peterson [:cpeterson]]

billm says WONTFIX.