921323 - crash in OpenGL@0x32af (MakeCurrent from OMTC)

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

This bug was filed from the Socorro interface and is 
report bp-0b8faad2-fab1-48a2-bf30-aa22e2130926.
=============================================================

Comment 1

[Matt Woodrow (:mattwoodrow)]

I think this a dup of bug 899044.

Lots of different stack traces, but always crashing in MakeCurrent.

Comment 2

[Benoit Girard (:BenWa)]

Probably but it points back to the fullscreen crash on 10.6 for which the test is disabled on 10.7+. We're not handling some of the widget changes to the windows and it seems to be messing with our context. It's preventing us from shipping OMTC on 10.6, giving us random test failures on orange and is a mac top crasher.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Can you explain how it leads to the fullscreen crash for 10.6? That's probably an important link, but I don't see it :)

Comment 4

[Benoit Girard (:BenWa)]

I'm going into 890997 but here it goes anyways:

Looks for call to DestroyNativeWindow:
http://mxr.mozilla.org/mozilla-central/search?string=DestroyNativeWindow

For example we call that from here:
http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsCocoaWindow.mm#539

I'm guessing destroying the window without syncing with the compositor that has a context attached to that window isn't good. I think we need to crack down on these problems cause they aren't only affecting 10.6. We also probably want to audit for more bad things else where. Perhaps we can find the internal symbol for when a context gets dettached from the widget and trace the points where it happens.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Right. I was more wondering if there was any evidence to suggest that is what is causing this crash bug.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Destroy compositor when we close the window

I can't find any correlation between the crashes and windows closing, but if that is the case, then this should help.

Comment 7

[Benoit Girard (:BenWa)]

Note that entering fullscreen or changing some the window properties recreate the window.

I'm not sure I understand how this patch is trying to help. Can you explain how you're hoping it will work. In the case where we temporarily destroy the widget we should aim to just unwind and rebind our GLContext from the old to the new widget instead of destroying and recreating all our resources.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Benoit Girard (:BenWa) from comment #7)
> Note that entering fullscreen or changing some the window properties
> recreate the window.
> 
> I'm not sure I understand how this patch is trying to help. Can you explain
> how you're hoping it will work. In the case where we temporarily destroy the
> widget we should aim to just unwind and rebind our GLContext from the old to
> the new widget instead of destroying and recreating all our resources.

That function gets called on nsChildView children of the nsCocoaWindow when we call DestroyNativeWindow. My change makes us synchronously tear down the compositor/GLContext.

I'm much more interested in not crashing than I am in optimal behaviour at this point.

Comment 9

[Benoit Girard (:BenWa)]

At the risk of being unreasonable (in which case I'd be ok if you found someone else to review this change), I feel like this change is just a shot in the dark without studying the problem for which we have can reliably reproduce. Not only is it a shot in the dark but it's a fairly invasive change with poor justification.

In the future it's going to be hard to maintain because it's a hack. Why is this compositor specific? Why isn't it going through the shutdown part like everything else? With this poor understanding we wont be able to remove/maintain this line because we don't even understand why it's there in the first place.

(In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> I'm much more interested in not crashing than I am in optimal behaviour at
> this point.

Then how much is this hurting us? It's a non trivial change. OMTC itself is just an optimization so if we prefer stability over performance (which we do) then we should disable OMTC over a hack that make performance worse and for which we're not sure fixes the initial problem.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Well I'm just entirely confused at this point.


(In reply to Benoit Girard (:BenWa) from comment #9)
> At the risk of being unreasonable (in which case I'd be ok if you found
> someone else to review this change), I feel like this change is just a shot
> in the dark without studying the problem for which we have can reliably
> reproduce. 

We can? I only see an intermittent orange bug, and some low volume crash reports, without useful comments. Being able to reproduce this would make life much easier.


> Not only is it a shot in the dark but it's a fairly invasive
> change with poor justification.
> 
> In the future it's going to be hard to maintain because it's a hack. Why is
> this compositor specific? Why isn't it going through the shutdown part like
> everything else? With this poor understanding we wont be able to
> remove/maintain this line because we don't even understand why it's there in
> the first place.

Isn't this what you asked me to do? You said that the crash reports pointed at it being the same issue as why we have it disabled on 10.6. I asked why you thought that, and you pointed to a particular set of calls that might cause these problems.

This patch should effectively fix that path, but as I said, I can't see any correlation between these MakeCurrent crashes and the 10.6 ones (they appear to be happening during a transition to/from fullscreen and crash in glDrawArrays). I can't see anything that would suggest windows being created/destroyed or fullscreen happening from the TBPL logs, or the crash reports.

So, if you *do* think this is the problem, then we should take the patch and find out if it changes anything. If you don't (which is fine, because I don't either), then we're back to the drawing board.

> 
> (In reply to Matt Woodrow (:mattwoodrow) from comment #8)
> > I'm much more interested in not crashing than I am in optimal behaviour at
> > this point.
> 
> Then how much is this hurting us? It's a non trivial change. OMTC itself is
> just an optimization so if we prefer stability over performance (which we
> do) then we should disable OMTC over a hack that make performance worse and
> for which we're not sure fixes the initial problem.

I don't think performance when switching to fullscreen or closing a window is in any way comparable to doubling scrolling throughput (which is what has been recorded for OMTC). One is an edge case, and the other is basically our biggest perf target.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Milan, do we have anyone that actually knows the cocoa widget code that could help out here?

I think OMTC is a big enough performance win that we should do everything we can to prevent it being turned off (to the point of living with a low-volume crasher, potentially).

As BenWa suggested, I'm just taking shots in the dark here with no STR and no knowledge of the coca widget code or API.

I suspect he's right that we're doing something wrong with our asynchronous interaction between GL and cocoa, but I haven't been able to find out exactly what thus far.

I can/will continue looking at this, any assistance would be appreciated though.

Comment 12

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

Markus, can you help?

Comment 13

[Markus Stange [:mstange]]

I'll try.

Comment 14

[Alex Keybl [:akeybl]]

Tracy, can you confirm this is a top crash?

Comment 15

[Tracy Walker [:tracy]]

overall, not a topcrasher. 
but for Mac its:
#2 on Fx25
#4 on Fx26
#12 on Fx27

Comment 16

[Benoit Girard (:BenWa)]

Ahh ok, I though that classified as a top crasher for the platform.

Comment 17

[Tracy Walker [:tracy]]

(In reply to Benoit Girard (:BenWa) from comment #16)
> Ahh ok, I though that classified as a top crasher for the platform.

Yes, it does. Sorry, I forgot to add the keyword when posting the stats.

Comment 18

[Kyle Wooten]

Thanks!

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Another bug that confirms we should consider having topcrash-$platform keywords :)

Comment 20

[Benoit Girard (:BenWa)]

Comment on attachment 810944 [details] [diff] [review]
Destroy compositor when we close the window

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

Let's wait and see if Markus can help out here before taking this on.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Looks like Matt's on this, assigning to him.

Comment 22

[Markus Stange [:mstange]]

I'm pretty confident that the work in bug 886999, bug 914437, bug 923114 and bug 923133 will fix this, so assigning this to myself.

Comment 23

[u279076]

Current Ranking (Mac only):
 * Release: N/A
 * Beta:    #2  (0.63%)
 * Aurora:  #8  (1.27%)
 * Nightly: #22 (0.25%)

Comment 24

[Benoit Girard (:BenWa)]

Me and mstange discussed this yesterday. We're going to let is slide for Beta since it's too late for the patches. This should be fixed on Nightly as of today or yesterday. We're going to uplift to Aurora after a few more days of baking.

Comment 25

[u279076]

Thanks Benoit. I'm going to call this wontfix for Fx25 given that info.

Comment 26

[Tracy Walker [:tracy]]

topcrash is being replaced by more precise keywords per https://bugzilla.mozilla.org/show_bug.cgi?id=927557#c3

Comment 27

[Marcia Knous [:marcia]]

Adding the signature that is showing up on 10.9 as well, since it appears to be the same crash.

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Benoit Girard (:BenWa) from comment #24)
> Me and mstange discussed this yesterday. We're going to let is slide for
> Beta since it's too late for the patches. This should be fixed on Nightly as
> of today or yesterday. We're going to uplift to Aurora after a few more days
> of baking.

Benoit - did anything land on nightly that can be nominated for uplift on this bug?

Comment 29

[Benoit Girard (:BenWa)]

Should we uplift your patches for this to aurora now?

Comment 30

[Lukas Blakk [:lsblakk] use ?needinfo]

Too late now for speculative uplifts to Beta, marking wontfix for 26.

Comment 31

[Markus Stange [:mstange]]

This was fixed for Firefox 27 by one or more patches in this push (probably those from bug 914437): https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=30c25c68bb3e

Comment 32

[u279076]

Flagging for verification based on crashstats.

Comment 33

[Ada [:adalucinet]]

No crashes were recorded on 27 beta based on Socorro in the last 3 weeks for:
- [@ OpenGL@0x32af ] signature: http://goo.gl/1tZToV
- [@ OpenGL@0x3ac8 ] signature: http://goo.gl/yqQsYm
- [@ OpenGL@0x4133 ] signature: http://goo.gl/NiYaV2

No crashes were recorded on Aurora 28.0a2 based on Socorro in the last 3 weeks for:
- first signature: http://goo.gl/kJRbAX
- second signature: http://goo.gl/Uetq2r
- third signature: http://goo.gl/Lpyiha