[@ nsChildView::GetLayerManager] crashes embedders on launch

RESOLVED FIXED

Status

()

defect
--
blocker
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: stuart.morgan+bugzilla, Assigned: mstange)

Tracking

({crash})

Trunk
All
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(crash signature)

Attachments

(1 attachment)

Reporter

Description

9 years ago
(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).

Updated

9 years ago
Keywords: crash
Summary: nsChildView::GetLayerManager crashes embedders on launch → [@ nsChildView::GetLayerManager] crashes embedders on launch
Reporter

Comment 1

9 years ago
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?
blocking2.0: --- → ?
Reporter

Updated

9 years ago
Severity: critical → blocker
Assignee

Comment 2

9 years ago
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?
This should have been fixed as part of bug 572283 (part 2)

Can you still reproduce the issue?
Assignee

Comment 4

9 years ago
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().
Assignee: nobody → mstange
blocking2.0: ? → final+
Posted patch Crash fixSplinter Review
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.
Attachment #458435 - Flags: review?(roc)
http://hg.mozilla.org/mozilla-central/rev/ed9773300a85
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsChildView::GetLayerManager]
You need to log in before you can comment on or make changes to this bug.