791521 - crash in mozilla::WebGLContext::LoseOldestWebGLContextIfLimitExceeded

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect)

Description

[Scoobidiver (away)]

There's one crash in 18.0a1/20120915: bp-d6bf337a-21b8-4f4a-a78a-9b1d62120916.

Signature 	mozilla::WebGLContext::LoseOldestWebGLContextIfLimitExceeded More Reports Search
UUID	d6bf337a-21b8-4f4a-a78a-9b1d62120916
Date Processed	2012-09-16 05:26:26
Uptime	45
Last Crash	30.1 minutes before submission
Install Age	3.0 hours since version was first installed.
Install Time	2012-09-16 02:25:57
Product	FennecAndroid
Version	18.0a1
Build ID	20120915030531
Release Channel	nightly
OS	Linux
OS Version	0.0.0 Linux 2.6.39.4-Eos3-g5bedc81 #1 SMP PREEMPT Sun Sep 9 22:20:24 EST 2012 armv7l
Build Architecture	arm
Build Architecture Info	
Crash Reason	SIGSEGV
Crash Address	0xc
App Notes 	
AdapterDescription: 'NVIDIA Corporation -- NVIDIA Tegra -- OpenGL ES 2.0 -- Model: MZ601, Product: RTCOREEU, Manufacturer: motorola, Hardware: stingray'
EGL? EGL+ GL Context? GL Context+ GL Layers? GL Layers+ WebGL! WebGL+ 
motorola MZ601
MOTO/RTCOREEU/umts_everest:3.2/H.6.5-17-3/1321319666:user/ota-rel-keys,release-keys
EMCheckCompatibility	False
Adapter Vendor ID	NVIDIA Corporation
Adapter Device ID	NVIDIA Tegra
Accessibility	Active
Device	motorola MZ601
Android API Version	16 (REL)
Android CPU ABI	armeabi-v7a

Frame 	Module 	Signature 	Source
0 	libxul.so 	mozilla::WebGLContext::LoseOldestWebGLContextIfLimitExceeded 	nsNodeInfoManager.h:94
1 	libxul.so 	mozilla::WebGLContext::SetDimensions 	WebGLContext.cpp:383
2 	libxul.so 	mozilla::widget::GfxInfoWebGL::GetWebGLParameter 	GfxInfoWebGL.cpp:41
3 	libxul.so 	mozilla::widget::GfxInfoBase::GetWebGLParameter 	GfxInfoBase.cpp:728
4 	libxul.so 	NS_InvokeByIndex_P 	xptcinvoke_arm.cpp:160
5 	libxul.so 	XPCWrappedNative::CallMethod 	XPCWrappedNative.cpp:3105
6 	libxul.so 	XPC_WN_CallMethod 	XPCWrappedNativeJSOps.cpp:1469
7 	libxul.so 	js::InvokeKernel 	jscntxtinlines.h:372
8 	libxul.so 	js::Interpret 	jsinterp.cpp:2454
9 	libxul.so 	js::RunScript 	jsinterp.cpp:324
10 	libxul.so 	js::Invoke 	jsinterp.cpp:378
11 	libxul.so 	JS_CallFunctionValue 	jsapi.cpp:5906
12 	libxul.so 	nsJSContext::CallEventHandler 	nsJSEnvironment.cpp:1926
13 	libxul.so 	nsJSEventListener::HandleEvent 	nsJSEventListener.cpp:188
14 	libxul.so 	nsEventListenerManager::HandleEventSubType 	nsEventListenerManager.cpp:846
15 	libxul.so 	nsEventListenerManager::HandleEventInternal 	nsEventListenerManager.cpp:919
16 	libxul.so 	nsEventTargetChainItem::HandleEvent 	nsEventListenerManager.h:144
17 	libxul.so 	nsEventTargetChainItem::HandleEventTargetChain 	nsEventDispatcher.cpp:317
18 	libxul.so 	nsEventDispatcher::Dispatch 	nsEventDispatcher.cpp:640
19 	libxul.so 	DocumentViewerImpl::LoadComplete 	nsDocumentViewer.cpp:1027
20 	libxul.so 	nsDocShell::EndPageLoad 	nsDocShell.cpp:6414 
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3AWebGLContext%3A%3ALoseOldestWebGLContextIfLimitExceeded

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: in SetDimensions return early if canvas is null

This is a null ptr deref in DocumentPrincipal() called from LoseOldestWebGLContextIfLimitExceeded() which can only come from one of the NodePrincipal() calls made there, which has to mean that NodePrincipal() was called on a null object.

These calls are (at line 676):

        nsIPrincipal *ourPrincipal = GetCanvas()->NodePrincipal();
        nsIPrincipal *theirPrincipal = contexts[i]->GetCanvas()->NodePrincipal();

We already check above that contexts[i]->GetCanvas() is non-null. So the only explanation is that this->GetCanvas() is null. Since we were called by SetDimensions, this means that SetDimensions was called on a WebGLContext which has a null canvas element pointer.

I'm not sure whether that is a bug in itself or is legitimate so by default I'll assume it's legitimate and we want to handle this case. This patch checks for null canvas in SetDimensions and returns early.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 661591 [details] [diff] [review]
in SetDimensions return early if canvas is null

This could use some feedback as to whether it is legitimate that WebGLContext::SetDimensions is called on a WebGLContext that has a null mCanvasElement pointer, and whether silentlty returning early in this case is the right thing to do.

Comment 3

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

Comment on attachment 661591 [details] [diff] [review]
in SetDimensions return early if canvas is null

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

This patch is OK, but I can't think of any reason why GetCanvas() would return null for a WebGLContext. As far as I can tell we only clear mCanvasElement during cycle collection Unlink, and this crash stack is not happening during cycle collection. (Maybe it's possible that during cycle collection something resurrects the canvas by adding a reference to it from a live object. I have no idea how that could happen.)

Comment 4

[Scoobidiver (away)]

STR on desktop:
1. Load https://www.khronos.org/registry/webgl/conformance-suites/1.0.1/webgl-conformance-tests.html
2. Click Run
3. Open the Troubleshooting Information tab
=> bp-89cfe854-ff89-4c86-826f-273a62120917

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

It turns out, this patch caused a crash in mochitest-other. On null canvas, we should return NS_ERROR_FAILURE, not NS_OK. This fixes the crash.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ac136b93a1a3

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac136b93a1a3

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 661591 [details] [diff] [review]
in SetDimensions return early if canvas is null

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined:  about:support crashes on certain drivers (at least Mesa)
Fix Landed on Version: 18
Risk to taking this patch (and alternatives if risky): this will regress about:support WebGL renderer, this field will become empty. I don't think that this matters much, as I don't expect that ESR users are very interested in WebGL and only a minority of those who do would notice that WebGL info is missing from about:support (WebGL itself keeps working)
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 661591 [details] [diff] [review]
in SetDimensions return early if canvas is null

Not a particularly critical fix for ESR users. No need to uplift.