crash in texImage2d custom quickstub while running WebGL conformance test suite [@ JSObject::getProperty]

RESOLVED FIXED

Status

()

Core
Canvas: WebGL
--
critical
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

({crash})

Trunk
x86_64
Linux
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

Backtrace:

(gdb) bt
#0  0x000000381e0a6afd in nanosleep () at ../sysdeps/unix/syscall-template.S:82
#1  0x000000381e0a6970 in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:138
#2  0x00007f3475becbb0 in ah_crap_handler (signum=11) at /home/bjacob/mozilla-central/toolkit/xre/nsSigHandlers.cpp:132
#3  0x00007f3475bf1951 in nsProfileLock::FatalSignalHandler (signo=11, info=0x7fff29ce3730, context=0x7fff29ce3600) at nsProfileLock.cpp:221
#4  <signal handler called>
#5  0x00007f3475109fd3 in JSObject::getProperty (this=0x0, cx=0x7f345d308400, id=139863078792260, vp=0x7fff29ce3bf8) at /home/bjacob/mozilla-central/js/src/jsobj.h:641
#6  0x00007f3475103d78 in JS_GetPropertyById (cx=0x7f345d308400, obj=0x0, id=139863078792260, vp=0x7fff29ce3bf8) at /home/bjacob/mozilla-central/js/src/jsapi.cpp:3472
#7  0x00007f3475103e60 in JS_GetProperty (cx=0x7f345d308400, obj=0x0, name=0x7f347774d3eb "width", vp=0x7fff29ce3bf8) at /home/bjacob/mozilla-central/js/src/jsapi.cpp:3485
#8  0x00007f3476a5e08e in nsICanvasRenderingContextWebGL_TexImage2D (cx=0x7f345d308400, argc=9, vp=0x7f34697fe7b8) at ../../../../dist/include/CustomQS_WebGL.h:369
#9  0x00007f347519f992 in js_Invoke (cx=0x7f345d308400, args=..., flags=0) at /home/bjacob/mozilla-central/js/src/jsinterp.cpp:546
#10 0x00007f347515fc3c in js_fun_apply (cx=0x7f345d308400, argc=9, vp=0x7f34697fe750) at /home/bjacob/mozilla-central/js/src/jsfun.cpp:2062
#11 0x00007f347518c56c in js_Interpret (cx=0x7f345d308400) at /home/bjacob/mozilla-central/js/src/jsops.cpp:2148
#12 0x00007f34751a00a0 in js_Invoke (cx=0x7f345d308400, args=..., flags=0) at /home/bjacob/mozilla-central/js/src/jsinterp.cpp:664
#13 0x00007f347515f6ad in js_fun_call (cx=0x7f345d308400, argc=3, vp=0x7f34697fe488) at /home/bjacob/mozilla-central/js/src/jsfun.cpp:1950
#14 0x00007f347518c56c in js_Interpret (cx=0x7f345d308400) at /home/bjacob/mozilla-central/js/src/jsops.cpp:2148
#15 0x00007f34751a00a0 in js_Invoke (cx=0x7f345d308400, args=..., flags=0) at /home/bjacob/mozilla-central/js/src/jsinterp.cpp:664
#16 0x00007f347515fc3c in js_fun_apply (cx=0x7f345d308400, argc=0, vp=0x7f34697fe278) at /home/bjacob/mozilla-central/js/src/jsfun.cpp:2062
#17 0x00007f347518c56c in js_Interpret (cx=0x7f345d308400) at /home/bjacob/mozilla-central/js/src/jsops.cpp:2148
#18 0x00007f34751a00a0 in js_Invoke (cx=0x7f345d308400, args=..., flags=0) at /home/bjacob/mozilla-central/js/src/jsinterp.cpp:664
#19 0x00007f3476a291f9 in nsXPCWrappedJSClass::CallMethod (this=0x7f3464efc1f0, wrapper=0x7f3465e67c00, methodIndex=3, info=0x7f346bc13318, nativeParams=0x7fff29ce6200)
    at /home/bjacob/mozilla-central/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1689
#20 0x00007f3476a20071 in nsXPCWrappedJS::CallMethod (this=0x7f3465e67c00, methodIndex=3, info=0x7f346bc13318, params=0x7fff29ce6200)
    at /home/bjacob/mozilla-central/js/src/xpconnect/src/xpcwrappedjs.cpp:570
#21 0x00007f34771afb27 in PrepareAndDispatch (self=0x7f34592380e0, methodIndex=3, args=0x7fff29ce63a0, gpregs=0x7fff29ce6320, fpregs=0x7fff29ce6350)
    at /home/bjacob/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:153
#22 0x00007f34771afbd5 in SharedStub () at /home/bjacob/mozilla-central/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:159
#23 0x00007f34762bdd55 in nsEventListenerManager::HandleEventSubType (this=0x7f3459f4cc10, aListenerStruct=0x7f3459f4cc80, aListener=0x7f34592380e0, aDOMEvent=0x7f345b3ffac0, 
    aCurrentTarget=0x7f345945d088, aPhaseFlags=6, aPusher=0x7fff29ce6880) at /home/bjacob/mozilla-central/content/events/src/nsEventListenerManager.cpp:1094
#24 0x00007f34762be214 in nsEventListenerManager::HandleEventInternal (this=0x7f3459f4cc10, aPresContext=0x7f345945c800, aEvent=0x7fff29ce69b0, aDOMEvent=0x7fff29ce6850, 
    aCurrentTarget=0x7f345945d088, aFlags=6, aEventStatus=0x7fff29ce6858, aPusher=0x7fff29ce6880) at /home/bjacob/mozilla-central/content/events/src/nsEventListenerManager.cpp:1190
#25 0x00007f34762e9c3d in nsEventListenerManager::HandleEvent (this=0x7f3459f4cc10, aPresContext=0x7f345945c800, aEvent=0x7fff29ce69b0, aDOMEvent=0x7fff29ce6850, aCurrentTarget=
    0x7f345945d088, aFlags=6, aEventStatus=0x7fff29ce6858, aPusher=0x7fff29ce6880) at /home/bjacob/mozilla-central/content/events/src/nsEventListenerManager.h:146
#26 0x00007f34762ea16d in nsEventTargetChainItem::HandleEvent (this=0x7f3468d67578, aVisitor=..., aFlags=6, aMayHaveNewListenerManagers=0, aPusher=0x7fff29ce6880)
    at /home/bjacob/mozilla-central/content/events/src/nsEventDispatcher.cpp:212
#27 0x00007f34762e7df2 in nsEventTargetChainItem::HandleEventTargetChain (this=0x7f3468d67150, aVisitor=..., aFlags=6, aCallback=0x0, aMayHaveNewListenerManagers=0, aPusher=
    0x7fff29ce6880) at /home/bjacob/mozilla-central/content/events/src/nsEventDispatcher.cpp:341
#28 0x00007f34762e8b89 in nsEventDispatcher::Dispatch (aTarget=0x7f345d308000, aPresContext=0x7f345945c800, aEvent=0x7fff29ce69b0, aDOMEvent=0x0, aEventStatus=0x7fff29ce69fc, 
    aCallback=0x0, aTargets=0x0) at /home/bjacob/mozilla-central/content/events/src/nsEventDispatcher.cpp:628
#29 0x00007f3475e99acc in DocumentViewerImpl::LoadComplete (this=0x7f345aabd140, aStatus=0) at /home/bjacob/mozilla-central/layout/base/nsDocumentViewer.cpp:1037
#30 0x00007f3476b0b9c8 in nsDocShell::EndPageLoad (this=0x7f345d307800, aProgress=0x7f345d307828, aChannel=0x7f3458be2e40, aStatus=0)
    at /home/bjacob/mozilla-central/docshell/base/nsDocShell.cpp:5766
#31 0x00007f3476b0b3ab in nsDocShell::OnStateChange (this=0x7f345d307800, aProgress=0x7f345d307828, aRequest=0x7f3458be2e40, aStateFlags=131088, aStatus=0)
    at /home/bjacob/mozilla-central/docshell/base/nsDocShell.cpp:5647
#32 0x00007f3476b38729 in nsDocLoader::FireOnStateChange (this=0x7f345d307800, aProgress=0x7f345d307828, aRequest=0x7f3458be2e40, aStateFlags=131088, aStatus=0)
    at /home/bjacob/mozilla-central/uriloader/base/nsDocLoader.cpp:1321
#33 0x00007f3476b37454 in nsDocLoader::doStopDocumentLoad (this=0x7f345d307800, request=0x7f3458be2e40, aStatus=0) at /home/bjacob/mozilla-central/uriloader/base/nsDocLoader.cpp:929
#34 0x00007f3476b3703d in nsDocLoader::DocLoaderIsEmpty (this=0x7f345d307800, aFlushLayout=1) at /home/bjacob/mozilla-central/uriloader/base/nsDocLoader.cpp:805
#35 0x00007f3476b36b6a in nsDocLoader::OnStopRequest (this=0x7f345d307800, aRequest=0x7f3458fc3ae0, aCtxt=0x0, aStatus=0)
---Type <return> to continue, or q <return> to quit---
    at /home/bjacob/mozilla-central/uriloader/base/nsDocLoader.cpp:700
#36 0x00007f3475c39679 in nsLoadGroup::RemoveRequest (this=0x7f345d5b7a80, request=0x7f3458fc3ae0, ctxt=0x0, aStatus=0)
    at /home/bjacob/mozilla-central/netwerk/base/src/nsLoadGroup.cpp:680
#37 0x00007f34761d204f in nsDocument::DoUnblockOnload (this=0x7f34593e2800) at /home/bjacob/mozilla-central/content/base/src/nsDocument.cpp:6949
#38 0x00007f34761d1e10 in nsDocument::UnblockOnload (this=0x7f34593e2800, aFireSync=1) at /home/bjacob/mozilla-central/content/base/src/nsDocument.cpp:6891
#39 0x00007f34761c6e47 in nsDocument::DispatchContentLoadedEvents (this=0x7f34593e2800) at /home/bjacob/mozilla-central/content/base/src/nsDocument.cpp:3892
#40 0x00007f34761e1f26 in nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run (this=0x7f345b0beca0) at ../../../dist/include/nsThreadUtils.h:347
#41 0x00007f3477192c8b in nsThread::ProcessNextEvent (this=0x7f3473638d70, mayWait=0, result=0x7fff29ce75ec) at /home/bjacob/mozilla-central/xpcom/threads/nsThread.cpp:547
#42 0x00007f347711f925 in NS_ProcessNextEvent_P (thread=0x7f3473638d70, mayWait=0) at nsThreadUtils.cpp:250
#43 0x00007f3476fe63ee in mozilla::ipc::MessagePump::Run (this=0x7f34736af800, aDelegate=0x7f34736d21c0) at /home/bjacob/mozilla-central/ipc/glue/MessagePump.cpp:118
#44 0x00007f347720247d in MessageLoop::RunInternal (this=0x7f34736d21c0) at /home/bjacob/mozilla-central/ipc/chromium/src/base/message_loop.cc:219
#45 0x00007f3477202402 in MessageLoop::RunHandler (this=0x7f34736d21c0) at /home/bjacob/mozilla-central/ipc/chromium/src/base/message_loop.cc:202
#46 0x00007f3477202393 in MessageLoop::Run (this=0x7f34736d21c0) at /home/bjacob/mozilla-central/ipc/chromium/src/base/message_loop.cc:176
#47 0x00007f3476e8bda1 in nsBaseAppShell::Run (this=0x7f346bcd4a20) at /home/bjacob/mozilla-central/widget/src/xpwidgets/nsBaseAppShell.cpp:175
#48 0x00007f3476be8a59 in nsAppStartup::Run (this=0x7f34695fa330) at /home/bjacob/mozilla-central/toolkit/components/startup/src/nsAppStartup.cpp:192
#49 0x00007f3475bdea1d in XRE_main (argc=4, argv=0x7fff29ce8258, aAppData=0x7f34736250f0) at /home/bjacob/mozilla-central/toolkit/xre/nsAppRunner.cpp:3619
#50 0x0000000000401f4f in main (argc=4, argv=0x7fff29ce8258) at /home/bjacob/mozilla-central/browser/app/nsBrowserApp.cpp:158
(Assignee)

Updated

8 years ago
Assignee: nobody → bjacob
Status: NEW → ASSIGNED
(Assignee)

Updated

8 years ago
OS: All → Linux
Hardware: All → x86_64

Updated

8 years ago
Severity: normal → critical
Keywords: crash
Summary: crash in texImage2d custom quickstub while running WebGL conformance test suite → crash in texImage2d custom quickstub while running WebGL conformance test suite [@ JSObject::getProperty]
(Assignee)

Comment 1

8 years ago
(note: i have a patch ready but hit another crash (unrelated) that i need to fix first)
(Assignee)

Comment 2

8 years ago
Created attachment 457157 [details] [diff] [review]
Fix WebGL custom quickstubs

This patch does 4 things:
* it fixes the crash reported here, the problem was that we weren't checking that the JS object wasn't null before passing it to JS_GetProperty;
* it does the same fix in texSubImage2D which had the same problem;
* it also fixes a bug (potential crash?) in BufferData where we weren't checking that a jsval was an object before converting it to object;
* it replaces usage of JSVAL_IS_PRIMITIVE by JSVAL_IS_NULL when we already know it's an object. Just to make our intent clear.
Attachment #457157 - Flags: review?(vladimir)
Comment on attachment 457157 [details] [diff] [review]
Fix WebGL custom quickstubs

>-    if (!JSVAL_IS_PRIMITIVE(argv[1])) {
>+    if (JSVAL_IS_OBJECT(argv[1]) &&
>+        !JSVAL_IS_NULL(argv[1])) {
>+

!IS_PRIMITIVE() is canonical for IS_OBJECT() && !IS_NULL, but ok.

>     if (!wa && !wb &&
>         !JS_ValueToECMAInt32(cx, argv[1], &size))
>     {
>+        xpc_qsThrowBadArg(cx, NS_ERROR_FAILURE, vp, 1);
>         return JS_FALSE;
>     }

Hm, so JS_ValueToECMAInt32 should have already set a JS error value (capital JS_* API), so returning JS_FALSE should be enough to propagate the error upwards.  Is this not the case?

>-    } else if (   argc == 7
>-               && JSVAL_IS_OBJECT(argv[6])
>-               && !JSVAL_IS_PRIMITIVE(argv[6]))
>+    } else if (argc == 7 &&
>+               JSVAL_IS_OBJECT(argv[6]) &&
>+               !JSVAL_IS_NULL(argv[6]))

The original code really just needed !JSVAL_IS_PRIMITIVE(), no need for the extra IS_OBJECT.

Actually, want to make things a lot clearer in this file, and just do something like:

#define IS_NON_NULL_OBJECT(x) (!JSVAL_IS_PRIMITIVE(x))

at the top of this file?  That way we can just use one macro and have it say exactly what we mean instead of !JSVAL_IS_PRIMITIVE.  In fact, I bet you could get that change into jsapi.h as a new macro as well.
(Assignee)

Comment 4

8 years ago
(In reply to comment #3)
> Comment on attachment 457157 [details] [diff] [review]
> Fix WebGL custom quickstubs
> 
> >-    if (!JSVAL_IS_PRIMITIVE(argv[1])) {
> >+    if (JSVAL_IS_OBJECT(argv[1]) &&
> >+        !JSVAL_IS_NULL(argv[1])) {
> >+
> 
> !IS_PRIMITIVE() is canonical for IS_OBJECT() && !IS_NULL, but ok.

Ah, I didn't know that --- I wrongly believed that a jsval could be neither primitive nor object. I checked in jsapi.h and indeed IS_PRIMITIVE is defined to what you say.

> 
> >     if (!wa && !wb &&
> >         !JS_ValueToECMAInt32(cx, argv[1], &size))
> >     {
> >+        xpc_qsThrowBadArg(cx, NS_ERROR_FAILURE, vp, 1);
> >         return JS_FALSE;
> >     }
> 
> Hm, so JS_ValueToECMAInt32 should have already set a JS error value (capital
> JS_* API), so returning JS_FALSE should be enough to propagate the error
> upwards.  Is this not the case?

OK, I didn't know about that.

> 
> >-    } else if (   argc == 7
> >-               && JSVAL_IS_OBJECT(argv[6])
> >-               && !JSVAL_IS_PRIMITIVE(argv[6]))
> >+    } else if (argc == 7 &&
> >+               JSVAL_IS_OBJECT(argv[6]) &&
> >+               !JSVAL_IS_NULL(argv[6]))
> 
> The original code really just needed !JSVAL_IS_PRIMITIVE(), no need for the
> extra IS_OBJECT.

I see, I guess that's what got me confused.

> 
> Actually, want to make things a lot clearer in this file, and just do something
> like:
> 
> #define IS_NON_NULL_OBJECT(x) (!JSVAL_IS_PRIMITIVE(x))
> 
> at the top of this file?  That way we can just use one macro and have it say
> exactly what we mean instead of !JSVAL_IS_PRIMITIVE.  In fact, I bet you could
> get that change into jsapi.h as a new macro as well.

Oh, I think the only problem was that I didn't know what primitive meant. Since there is already IS_PRIMITIVE, let's just use that.
(Assignee)

Comment 5

8 years ago
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 457157 [details] [diff] [review] [details]
> > 
> > >     if (!wa && !wb &&
> > >         !JS_ValueToECMAInt32(cx, argv[1], &size))
> > >     {
> > >+        xpc_qsThrowBadArg(cx, NS_ERROR_FAILURE, vp, 1);
> > >         return JS_FALSE;
> > >     }
> > 
> > Hm, so JS_ValueToECMAInt32 should have already set a JS error value (capital
> > JS_* API), so returning JS_FALSE should be enough to propagate the error
> > upwards.  Is this not the case?
> 
> OK, I didn't know about that.

From a quick look at the code, I couldn't see it doing that?

Anyway, that would be a rather bad thing for us, because there is no way that

     JS_ValueToECMAInt32(cx, argv[1], &size)

could generate a useful error message mentioning explicitly "argument 1", right? So I hope it's not the case.
(Assignee)

Comment 6

8 years ago
Created attachment 457331 [details] [diff] [review]
Fix WebGL custom quickstubs

Here's a new version relying on JSVAL_IS_PRIMITIVE, but still not assuming that JS_ValueToECMAInt32 throws errors for us.
Attachment #457157 - Attachment is obsolete: true
Attachment #457331 - Flags: review?(vladimir)
Attachment #457157 - Flags: review?(vladimir)
Yeah, "null" is both a primitive and an object, so it'll be caught by JSVAL_IS_PRIMITIVE.  Anything not there in a non-null object.

However, for JS_ValueToECMAInt32, you're actually required to -not- report an error since one was already reported -- it won't mention "argument 1" etc., but that's not required.  Otherwise you'll get double errors and potentially multiple exceptions in flight.  It reports the error only in the case where it has to ask an object for a value (which is the only case where it can fail), deep inside jsobj.
(Assignee)

Comment 8

8 years ago
Created attachment 457356 [details] [diff] [review]
Fix WebGL custom quickstubs

OK, here's a new version no longer throwing errors in this case.
Attachment #457331 - Attachment is obsolete: true
Attachment #457356 - Flags: review?(vladimir)
Attachment #457331 - Flags: review?(vladimir)
Comment on attachment 457356 [details] [diff] [review]
Fix WebGL custom quickstubs

Looks good, some style nits before you check in:

>+    } else if (argc == 7 &&
>+               !JSVAL_IS_PRIMITIVE(argv[6]))
>         {

^ The indentation here went all crazy

>-    } else if (argc > 5 && JSVAL_IS_OBJECT(argv[5])) {
>+    } else if (argc > 5 &&
>+               !JSVAL_IS_PRIMITIVE(argv[5]) ) {
>+

no space before final ), and { on its own line pls

>-    } else if (argc > 8 && JSVAL_IS_OBJECT(argv[8])) {
>+    } else if (argc > 8 && JSVAL_IS_OBJECT(argv[8])) { // here, we allow null !

If you're putting the PRIMITIVE checks on their own line above, why not move the OBJECT the same?

>-    if (argc > 6 && JSVAL_IS_OBJECT(argv[6])) {
>+    if (argc > 6 &&
>+        !JSVAL_IS_PRIMITIVE(argv[6]) ) {

Same extra space and newline { here

>-    } else if (argc > 8 && JSVAL_IS_OBJECT(argv[8])) {
>+    } else if (argc > 8 &&
>+               !JSVAL_IS_PRIMITIVE(argv[8])) {
>+

etc.
Attachment #457356 - Flags: review?(vladimir) → review+
http://hg.mozilla.org/mozilla-central/rev/974d991f537a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Crash Signature: [@ JSObject::getProperty]
You need to log in before you can comment on or make changes to this bug.