Closed Bug 861490 Opened 9 years ago Closed 9 years ago

crash in nsChildView::MaybeDrawRoundedBottomCorners with OMTC

Categories

(Core :: Widget: Cocoa, defect)

23 Branch
x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox22 --- unaffected
firefox23 --- verified

People

(Reporter: scoobidiver, Assigned: nrc)

References

Details

(Keywords: crash, regression, Whiteboard: [startupcrash])

Crash Data

Attachments

(1 file, 1 obsolete file)

It first showed up in 23.0a1/20130412 and seems an OMTC regression based on comments. The regression range might be (setting different from its default value because of http://featherweightmusings.blogspot.co.at/2013/04/the-layers-refactoring-has-landed.html):
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2949e808ed33&tochange=7b8ed29c6bc0
It might be a regression from bug 858550.

Signature 	nsChildView::MaybeDrawRoundedBottomCorners(mozilla::layers::LayerManagerOGL*, nsIntRect) More Reports Search
UUID	3da9ac5c-a7a7-4dfd-86ba-eba862130413
Date Processed	2013-04-13 06:51:00
Uptime	1
Last Crash	22 seconds before submission
Install Age	11.1 hours since version was first installed.
Install Time	2013-04-12 19:42:11
Product	Firefox
Version	23.0a1
Build ID	20130412030828
Release Channel	nightly
OS	Mac OS X
OS Version	10.8.3 12D78
Build Architecture	amd64
Build Architecture Info	family 6 model 58 stepping 9
Crash Reason	EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash Address	0x0
User Comments	Testing: "layers.offmainthreadcomposition.enabled" set to true as per: http://featherweightmusings.blogspot.co.at/2013/04/the-layers-refactoring-has-landed.html nope… It's dead now :D
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x 166GL Layers! GL Context? GL Context+ GL Layers+ 
Processor Notes 	sp-processor02.phx1.mozilla.com_20236:2012; exploitability tool failed: 127
EMCheckCompatibility	True
Adapter Vendor ID	0x8086
Adapter Device ID	0x 166

Frame 	Module 	Signature 	Source
0 	XUL 	nsChildView::MaybeDrawRoundedBottomCorners 	widget/cocoa/nsChildView.mm:1990
1 	libobjc.A.dylib 	lookUpMethod 	
2 	XUL 	nsChildView::DrawWindowOverlay 	widget/cocoa/nsChildView.mm:1870
3 	XUL 	XUL@0x1599480 	
4 	XUL 	mozilla::layers::LayerManagerComposite::Render 	LayerManagerComposite.cpp:290
5 	XUL 	mozilla::layers::LayerManager::GetScrollableLayers 	obj-firefox/x86_64/dist/include/nsTArray-inl.h:234
6 	XUL 	XUL@0x1599480 	
7 	XUL 	mozilla::layers::LayerManagerComposite::EndTransaction 	LayerManagerComposite.cpp:185
8 	XUL 	mozilla::layers::LayerManagerComposite::EndEmptyTransaction 	LayerManagerComposite.cpp:150
9 	XUL 	mozilla::layers::CompositorParent::Composite 	gfx/layers/ipc/CompositorParent.cpp:570
10 	XUL 	MessageLoop::DeferOrRunPendingTask 	ipc/chromium/src/base/message_loop.cc:334
11 	XUL 	MessageLoop::DoWork 	ipc/chromium/src/base/message_loop.cc:442
12 	XUL 	base::MessagePumpDefault::Run 	message_pump_default.cc:23
13 	XUL 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:216
14 	XUL 	base::Thread::ThreadMain 	thread.cc:156 

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsChildView%3A%3AMaybeDrawRoundedBottomCorners%28mozilla%3A%3Alayers%3A%3ALayerManagerOGL*%2C+nsIntRect%29
Whiteboard: [startupcrash]
(In reply to Scoobidiver from comment #0)
> It first showed up in 23.0a1/20130412
Seen earlier in 23.0a1/20130410065939.
Summary: crash in nsChildView::MaybeDrawRoundedBottomCorners → crash in nsChildView::MaybeDrawRoundedBottomCorners with OMTC
I expect this is because we expect aManager to be LayerManagerOGL, but it is now a LayerManagerComposite.
Blocks: 756601
What a horror show. There is no nice fix for this. If we ever get GL to be OMTC only, then this problem will go away. For now I think the only solution is large amounts of duplicated code. (FWIW, the nice fix is to have LayerManagerOGL backed by CompositorOGL which was working nicely on the graphics branch for a while, but which was deemed too risky to land, probably right about the risk though :-( ).
Assignee: nobody → ncameron
I'm surprised that many people are using mac OMTC. It was only designed to speed up development of other features to bridge the gap ofpoor devtools on mobile which has been largely addressed now.
(In reply to Benoit Girard (:BenWa) from comment #4)
> I'm surprised that many people are using mac OMTC. It was only designed to
> speed up development of other features to bridge the gap ofpoor devtools on
> mobile which has been largely addressed now.

I doubt it is many. I made a request on my blog for help testing the layers refactoring, seems like at least one person on a mac answered the call.
Attached patch patch (obsolete) — Splinter Review
There is another use of LayerManagerOGL in nsChildView.nm which I am suspicious of - in DrawUsingOpenGL, but I didn't seem to hit that path when I was testing and since I didn't understand when it should be hit I was wary of changing it, but perhaps that also needs fixing up.
Attachment #737818 - Flags: review?(matt.woodrow)
Attachment #737818 - Flags: review?(matt.woodrow)
(In reply to Nick Cameron [:nrc] from comment #7)
> Created attachment 737818 [details] [diff] [review]
> patch
> 
> There is another use of LayerManagerOGL in nsChildView.nm which I am
> suspicious of - in DrawUsingOpenGL, but I didn't seem to hit that path when
> I was testing and since I didn't understand when it should be hit I was wary
> of changing it, but perhaps that also needs fixing up.

Seems to leak :-(
Attached patch patchSplinter Review
still need to decide what to do about the potential race in ShowsResizeIndicator
Attachment #737818 - Attachment is obsolete: true
Attachment #737890 - Flags: review?(matt.woodrow)
Attachment #737890 - Flags: review?(matt.woodrow) → review+
Could we trigger code to run every time that the result of ShowsResizeIndicator might change?

Then we could compute this and cache it on the main thread, and just read it from the compositor thread.
(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Could we trigger code to run every time that the result of
> ShowsResizeIndicator might change?
> 
> Then we could compute this and cache it on the main thread, and just read it
> from the compositor thread.

I think we should file this as a follow up bug - we only want to stop crashing here, and Mac OMTC is still very experimental - this patch just takes us back to how we were before the refactoring. We should block Mac OMTC on it though. This might get complicated and I don't want to wait for that to land this.

Is that OK with you?
Absolutely, that was the intent of giving the patch r+ as is.
(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> Absolutely, that was the intent of giving the patch r+ as is.

bug 862556
Unfortunately this has created a shutdown crash with debug builds (and OMTC enabled).

The TextureImage object we cache on nsChildView is created from the compositor thread, but released from the main thread.

We don't have threadsafe refcounting for this type (and probably shouldn't), so this hits an assertion.
https://hg.mozilla.org/mozilla-central/rev/e39cf91fd52b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Is there anything QA can do to verify this is fixed?
Checking the crash stats I see no crashes for the last 4 weeks. Marking verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.