570314 - [@ nsChildView::GetLayerManager] crashes embedders on launch

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Stuart Morgan]

(Copying from bug 561957, since this needs a bug tracking it)

>+  nsCocoaWindow* window = GetXULWindowWidget();
>+  if (window->GetAcceleratedRendering() != mUseAcceleratedRendering) {

Not all Gecko browsers are XUL-based; this code crashes Camino on launch. The
return value of GetXULWindowWidget will always be null for embedding.

I don't have enough context to know if an additional check is all that is
required, or if there need to be deeper changes to make this embedding-friendly
(it seems like latter though, otherwise mUseAcceleratedRendering will always be
false, which is presumably not ideal).

Comment 1

[Stuart Morgan]

Really? Almost an entire month that clearly incorrect code has been known to render Mac embedding of Gecko 1.9.3 completely impossible, and nobody cares at all?

Josh, if nobody is going to fix this broken code, as module owner could you revert the change?

Comment 2

[Markus Stange [:mstange]]

I don't see evidence that "nobody is going to", I just see that "nobody has yet". I'm sure Matt will happily add the null check as soon as he finds time.

(In reply to comment #0)
> (it seems like latter though, otherwise mUseAcceleratedRendering will always be
> false, which is presumably not ideal).

At the moment it's only true for full-screen video. How does / will that work in Camino? Also via a full-screen XUL window?

Comment 3

[Matt Woodrow (:mattwoodrow)]

This should have been fixed as part of bug 572283 (part 2)

Can you still reproduce the issue?

Comment 4

[Markus Stange [:mstange]]

Hmm, that makes us return nsnull - shouldn't we return a BasicLayerManager, i.e. nsBaseWidget::GetLayerManager()? I think returning nsnull makes us crash in drawRect: mGeckoChild->GetLayerManager()->GetBackendType().

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Crash fix

Well spotted Marcus!

This should be the correct behavior to prevent crashes. Embedders will still need their own way to set the accelerated flag on a window, but I think that can be a separate issue.

Comment 6

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

http://hg.mozilla.org/mozilla-central/rev/ed9773300a85