Closed Bug 804558 Opened 7 years ago Closed 7 years ago

toSource on a chrome script trips JS::AssertCanGC()

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: peterv, Assigned: Benjamin)

References

Details

Attachments

(2 files)

Simply doing

let f = function (){};
f.toSource();

in a browser-chrome test trips the assert in JS::AssertCanGC().

The patch adds a browser-chrome test in dom/tests/browser/browser_xhr_toSource.js that shows this. I'm trying to add a test in bug 778152, but it hits this problem.
I am running into crashes on osx now with toolkit/components/social/test/browser/browser_frameworker.js and browser_notifications.js that also points to JS::AssertCanGC().  Thoses tests also use toSource to generate a data url.  Mark Hammond has the same crash on windows, though he hasn't got the stacktrace yet.
Terrence, could you look into this?
Assignee: general → terrence
Peter, do you have a stack, by chance?
Here is a callstack from MSVC:

>	mozjs.dll!JS::AssertCanGC()  Line 767 + 0x23 bytes	C++
 	mozjs.dll!js::Shape::search(JSContext * cx, js::Shape * start, jsid id, js::Shape * * * pspp, bool adding)  Line 1062	C++
 	mozjs.dll!js::ObjectImpl::nativeLookup(JSContext * cx, jsid id)  Line 265 + 0x1c bytes	C++
 	mozjs.dll!LookupPropertyWithFlagsInline(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<jsid> id, unsigned int flags, JS::MutableHandle<JSObject *> objp, JS::MutableHandle<js::Shape *> propp)  Line 4098 + 0x22 bytes	C++
 	mozjs.dll!js_GetPropertyHelperInline(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<JSObject *> receiver, jsid id_, unsigned int getHow, JS::MutableHandle<JS::Value> vp)  Line 4311 + 0x41 bytes	C++
 	mozjs.dll!js::GetPropertyHelper(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<jsid> id, unsigned int getHow, JS::MutableHandle<JS::Value> vp)  Line 4399 + 0x28 bytes	C++
 	mozjs.dll!js::GetMethod(JSContext * cx, JS::Handle<JSObject *> obj, JS::Handle<jsid> id, unsigned int getHow, JS::MutableHandle<JS::Value> vp)  Line 4448 + 0x19 bytes	C++
 	mozjs.dll!JS_GetMethodById(JSContext * cx, JSObject * objArg, jsid idArg, JSObject * * objp, JS::Value * vp)  Line 4300 + 0x35 bytes	C++
 	mozjs.dll!JS_GetMethod(JSContext * cx, JSObject * objArg, const char * name, JSObject * * objp, JS::Value * vp)  Line 4313 + 0x2e bytes	C++
 	xul.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper, unsigned short methodIndex, const XPTMethodDescriptor * info, nsXPTCMiniVariant * nativeParams)  Line 1243 + 0x29 bytes	C++
 	xul.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex, const XPTMethodDescriptor * info, nsXPTCMiniVariant * params)  Line 581	C++
 	xul.dll!PrepareAndDispatch(nsXPTCStubBase * self, unsigned int methodIndex, unsigned int * args, unsigned int * stackBytesToPop)  Line 85 + 0x21 bytes	C++
 	xul.dll!SharedStub()  Line 113	C++
 	xul.dll!nsExternalHelperAppService::GetTypeFromExtension(const nsACString_internal & aFileExt, nsACString_internal & aContentType)  Line 2578 + 0x2a bytes	C++
 	xul.dll!CopyUTF16toUTF8(const wchar_t * aSource, nsACString_internal & aDest)  Line 61 + 0xd bytes	C++
 	xul.dll!nsExternalHelperAppService::AddRef()  Line 515 + 0xdc bytes	C++
 	xul.dll!nsComponentManagerImpl::GetServiceByContractID(const char * aContractID, const nsID & aIID, void * * result)  Line 1345 + 0x33 bytes	C++
 	xul.dll!nsGetServiceByContractIDWithError::operator()(const nsID & aIID, void * * aInstancePtr)  Line 256 + 0x13 bytes	C++
 	xul.dll!nsCOMPtr<nsIMIMEService>::assign_from_gs_contractid_with_error(const nsGetServiceByContractIDWithError & gs, const nsID & aIID)  Line 1197	C++
 	xul.dll!nsCOMPtr<nsIMIMEService>::nsCOMPtr<nsIMIMEService>(const nsGetServiceByContractIDWithError & gs)  Line 595	C++
 	092b2750()	
 	xul.dll!5b6a098f() 	
 	xul.dll!nsExternalHelperAppService::AddRef() 	Unknown
OK.  So bug 794667 definitely added the AssertCanGC to Shape::search.  Presumably because we can hashify the shapes under there?

What confuses me is where the AutoAssertNoGC is on the satack there.  I don't see it in nativeLookup or LookupPropertyWithFlagsInline and before that we're up in jsapi land where we shouldn't really be asserting no-GC because anything could happen there.

I assume we're getting into the MIME service stuff because something called GetTypeFromExtension, but the stack after that point is bogus...  I'm guessing that happens somewhere under the loadSource() callback for the scropt, but why are we under and AutoAssertNoGC there?
s/scropt/script/, of course.
Another bit of apparent bogusness is that Shane and I can trip this crash on a clean m-c tree simply by running the browser-chrome test toolkit/components/social/test/browser/browser_frameworker.js - but TBPL is showing no such symptoms.
I can reproduce this on osx by just running js/xpconnect/tests/chrome/test_chrometoSource.xul
Here's a better stack:

#0  0x00007ffff1c26d76 in JS::AssertCanGC () at ../../dist/include/gc/Root.h:767
#1  0x00007ffff381aba4 in js::Shape::search (cx=0x7fffe6e48280, start=0x7fffd0806d80, id=..., pspp=0x7fffffff6cf8, adding=false) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsscope.h:1059
#2  0x00007ffff39111be in js::ObjectImpl::nativeLookup (this=0x7fffdd1faa00, cx=0x7fffe6e48280, id=...) at /home/benjamin/dev/repos/mozilla/inbound/js/src/vm/ObjectImpl.cpp:265
#3  0x00007ffff37b4106 in LookupPropertyWithFlagsInline (cx=0x7fffe6e48280, obj=..., id=..., flags=1, objp=..., propp=...) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsobj.cpp:4098
#4  0x00007ffff37b5456 in js_GetPropertyHelperInline (cx=0x7fffe6e48280, obj=..., receiver=..., id_=..., getHow=0, vp=...) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsobj.cpp:4311
#5  0x00007ffff37b5bab in js::GetPropertyHelper (cx=0x7fffe6e48280, obj=..., id=..., getHow=0, vp=...) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsobj.cpp:4399
#6  0x00007ffff37b5efa in js::GetMethod (cx=0x7fffe6e48280, obj=..., id=..., getHow=0, vp=...) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsobj.cpp:4448
#7  0x00007ffff368e294 in JS_GetMethodById (cx=0x7fffe6e48280, objArg=0x7fffdd1faa00, idArg=..., objp=0x7fffffff7618, vp=0x7fffffff7620) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsapi.cpp:4300
#8  0x00007ffff368e3ac in JS_GetMethod (cx=0x7fffe6e48280, objArg=0x7fffdd1faa00, name=0x7fffe7aebdd0 "getTypeFromExtension", objp=0x7fffffff7618, vp=0x7fffffff7620) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsapi.cpp:4313
#9  0x00007ffff21b72e4 in nsXPCWrappedJSClass::CallMethod (this=0x7fffcfb84f60, wrapper=0x7fffcfb8c200, methodIndex=8, info=0x7fffe7aebd18, nativeParams=0x7fffffff7910)
    at /home/benjamin/dev/repos/mozilla/inbound/js/xpconnect/src/XPCWrappedJSClass.cpp:1243
#10 0x00007ffff21ad19e in nsXPCWrappedJS::CallMethod (this=0x7fffcfb8c200, methodIndex=8, info=0x7fffe7aebd18, params=0x7fffffff7910) at /home/benjamin/dev/repos/mozilla/inbound/js/xpconnect/src/XPCWrappedJS.cpp:580
#11 0x00007ffff2b29b64 in PrepareAndDispatch (self=0x7fffcfb8b320, methodIndex=8, args=0x7fffffff7aa0, gpregs=0x7fffffff7a20, fpregs=0x7fffffff7a50)
    at /home/benjamin/dev/repos/mozilla/inbound/xpcom/reflect/xptcall/src/md/unix/xptcstubs_x86_64_linux.cpp:121
#12 0x00007ffff2b28d27 in SharedStub () from /home/benjamin/dev/repos/mozilla/inbound/obj-x86_64-unknown-linux-gnu/dist/bin/libxul.so
#13 0x00007ffff22e705c in nsExternalHelperAppService::GetTypeFromExtension (this=0x7fffdf9ffdc0, aFileExt=..., aContentType=...) at /home/benjamin/dev/repos/mozilla/inbound/uriloader/exthandler/nsExternalHelperAppService.cpp:2578
#14 0x00007ffff22e7b6e in nsExternalHelperAppService::GetTypeFromFile (this=0x7fffdf9ffdc0, aFile=0x7fffe077a580, aContentType=...) at /home/benjamin/dev/repos/mozilla/inbound/uriloader/exthandler/nsExternalHelperAppService.cpp:2723
#15 0x00007ffff1029f48 in nsFileChannel::MakeFileInputStream (this=0x7fffcd260820, file=0x7fffe077a580, stream=..., contentType=...) at /home/benjamin/dev/repos/mozilla/inbound/netwerk/protocol/file/nsFileChannel.cpp:298
#16 0x00007ffff102a50f in nsFileChannel::OpenContentStream (this=0x7fffcd260820, async=false, result=0x7fffffff8050, channel=0x7fffffff7fb0) at /home/benjamin/dev/repos/mozilla/inbound/netwerk/protocol/file/nsFileChannel.cpp:367
#17 0x00007ffff0f40b61 in nsBaseChannel::Open (this=0x7fffcd260820, result=0x7fffffff8050) at /home/benjamin/dev/repos/mozilla/inbound/netwerk/base/src/nsBaseChannel.cpp:558
#18 0x00007ffff219d357 in ReadSourceFromFilename (cx=0x7fffe6e48280, filename=0x7fffdf981bf1 "chrome://mochitests/content/browser/dom/tests/browser/browser_xhr_toSource.js", src=0x7fffffff8210, len=0x7fffffff820c)
    at /home/benjamin/dev/repos/mozilla/inbound/js/xpconnect/src/XPCJSRuntime.cpp:2283
#19 0x00007ffff219d89c in SourceHook (cx=0x7fffe6e48280, script=0x7fffcca72bb0, src=0x7fffffff8210, length=0x7fffffff820c) at /home/benjamin/dev/repos/mozilla/inbound/js/xpconnect/src/XPCJSRuntime.cpp:2340
#20 0x00007ffff3828faa in JSScript::loadSource (this=0x7fffcca72bb0, cx=0x7fffe6e48280, worked=0x7fffffff836f) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsscript.cpp:1053
#21 0x00007ffff3712f0e in js::FunctionToString (cx=0x7fffe6e48280, fun=..., bodyOnly=false, lambdaParen=false) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsfun.cpp:647
#22 0x00007ffff3713c4a in fun_toStringHelper (cx=0x7fffe6e48280, obj=..., indent=32768) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsfun.cpp:784
#23 0x00007ffff371403f in fun_toSource (cx=0x7fffe6e48280, argc=0, vp=0x7fffe3fff200) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsfun.cpp:821
#24 0x00007ffff376ca4a in js::CallJSNative (cx=0x7fffe6e48280, native=0x7ffff3713ee2 <fun_toSource(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jscntxtinlines.h:364
#25 0x00007ffff3776271 in js::InvokeKernel (cx=0x7fffe6e48280, args=..., construct=js::NO_CONSTRUCT) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsinterp.cpp:367
#26 0x00007ffff377e147 in js::Interpret (cx=0x7fffe6e48280, entryFrame=0x7fffe3fff060, interpMode=js::JSINTERP_NORMAL) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsinterp.cpp:2369
#27 0x00007ffff3775eac in js::RunScript (cx=0x7fffe6e48280, script=..., fp=0x7fffe3fff060) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsinterp.cpp:324
#28 0x00007ffff3776396 in js::InvokeKernel (cx=0x7fffe6e48280, args=..., construct=js::NO_CONSTRUCT) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsinterp.cpp:379
#29 0x00007ffff36a7f6a in js::Invoke (cx=0x7fffe6e48280, args=..., construct=js::NO_CONSTRUCT) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsinterp.h:109
#30 0x00007ffff37150ff in js::CallOrConstructBoundFunction (cx=0x7fffe6e48280, argc=1, vp=0x7fffe3fff020) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsfun.cpp:1100
#31 0x00007ffff376ca4a in js::CallJSNative (cx=0x7fffe6e48280, native=0x7ffff3714e7f <js::CallOrConstructBoundFunction(JSContext*, unsigned int, JS::Value*)>, args=...)
    at /home/benjamin/dev/repos/mozilla/inbound/js/src/jscntxtinlines.h:364
#32 0x00007ffff3776271 in js::InvokeKernel (cx=0x7fffe6e48280, args=..., construct=js::NO_CONSTRUCT) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsinterp.cpp:367
#33 0x00007ffff36a7f6a in js::Invoke (cx=0x7fffe6e48280, args=..., construct=js::NO_CONSTRUCT) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsinterp.h:109
#34 0x00007ffff377665f in js::Invoke (cx=0x7fffe6e48280, thisv=..., fval=..., argc=1, argv=0x7fffced13220, rval=0x7fffffffa9b0) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsinterp.cpp:412
#35 0x00007ffff36963a3 in JS_CallFunctionValue (cx=0x7fffe6e48280, objArg=0x7fffd08a2060, fval=..., argc=1, argv=0x7fffced13220, rval=0x7fffffffa9b0) at /home/benjamin/dev/repos/mozilla/inbound/js/src/jsapi.cpp:5884
#36 0x00007ffff1b20e3d in nsJSContext::CallEventHandler (this=0x7fffcfbe8f00, aTarget=0x7fffcfdb9800, aScope=0x7fffd08a2060, aHandler=0x7fffccc2e500, aargv=0x7fffe1060d60, arv=0x7fffffffaab0)
    at /home/benjamin/dev/repos/mozilla/inbound/dom/base/nsJSEnvironment.cpp:1913
#37 0x00007ffff1b656ad in nsGlobalWindow::RunTimeoutHandler (this=0x7fffcfdb9800, aTimeout=0x7fffe0714dd0, aScx=0x7fffcfbe8f00) at /home/benjamin/dev/repos/mozilla/inbound/dom/base/nsGlobalWindow.cpp:9750
#38 0x00007ffff1b65f44 in nsGlobalWindow::RunTimeout (this=0x7fffcfdb9800, aTimeout=0x7fffe0714dd0) at /home/benjamin/dev/repos/mozilla/inbound/dom/base/nsGlobalWindow.cpp:9999
#39 0x00007ffff1b66adc in nsGlobalWindow::TimerCallback (aTimer=0x7fffcfcd1320, aClosure=0x7fffe0714dd0) at /home/benjamin/dev/repos/mozilla/inbound/dom/base/nsGlobalWindow.cpp:10266
#40 0x00007ffff2b048e4 in nsTimerImpl::Fire (this=0x7fffcfcd1320) at /home/benjamin/dev/repos/mozilla/inbound/xpcom/threads/nsTimerImpl.cpp:472
#41 0x00007ffff2b04d07 in nsTimerEvent::Run (this=0x7fffe7a020c8) at /home/benjamin/dev/repos/mozilla/inbound/xpcom/threads/nsTimerImpl.cpp:555
#42 0x00007ffff2afc9a9 in nsThread::ProcessNextEvent (this=0x7ffff6d75d50, mayWait=false, result=0x7fffffffaf7f) at /home/benjamin/dev/repos/mozilla/inbound/xpcom/threads/nsThread.cpp:620
#43 0x00007ffff2a8c14f in NS_ProcessNextEvent_P (thread=0x7ffff6d75d50, mayWait=false) at /home/benjamin/dev/repos/mozilla/inbound/obj-x86_64-unknown-linux-gnu/xpcom/build/nsThreadUtils.cpp:220
#44 0x00007ffff2804600 in mozilla::ipc::MessagePump::Run (this=0x7ffff6dc5340, aDelegate=0x7ffff6ddb0b0) at /home/benjamin/dev/repos/mozilla/inbound/ipc/glue/MessagePump.cpp:82
#45 0x00007ffff2b533e7 in MessageLoop::RunInternal (this=0x7ffff6ddb0b0) at /home/benjamin/dev/repos/mozilla/inbound/ipc/chromium/src/base/message_loop.cc:215
#46 0x00007ffff2b53378 in MessageLoop::RunHandler (this=0x7ffff6ddb0b0) at /home/benjamin/dev/repos/mozilla/inbound/ipc/chromium/src/base/message_loop.cc:208
#47 0x00007ffff2b53351 in MessageLoop::Run (this=0x7ffff6ddb0b0) at /home/benjamin/dev/repos/mozilla/inbound/ipc/chromium/src/base/message_loop.cc:182
#48 0x00007ffff2679920 in nsBaseAppShell::Run (this=0x7fffe6eb3e10) at /home/benjamin/dev/repos/mozilla/inbound/widget/xpwidgets/nsBaseAppShell.cpp:163
#49 0x00007ffff2399048 in nsAppStartup::Run (this=0x7fffe6f0dd30) at /home/benjamin/dev/repos/mozilla/inbound/toolkit/components/startup/nsAppStartup.cpp:290
#50 0x00007ffff0ef98e3 in XREMain::XRE_mainRun (this=0x7fffffffb4d0) at /home/benjamin/dev/repos/mozilla/inbound/toolkit/xre/nsAppRunner.cpp:3799
#51 0x00007ffff0ef9bc8 in XREMain::XRE_main (this=0x7fffffffb4d0, argc=5, argv=0x7fffffffd938, aAppData=0x635c20) at /home/benjamin/dev/repos/mozilla/inbound/toolkit/xre/nsAppRunner.cpp:3866
#52 0x00007ffff0ef9e05 in XRE_main (argc=5, argv=0x7fffffffd938, aAppData=0x635c20, aFlags=0) at /home/benjamin/dev/repos/mozilla/inbound/toolkit/xre/nsAppRunner.cpp:3941
#53 0x0000000000402d0a in do_main (argc=5, argv=0x7fffffffd938) at /home/benjamin/dev/repos/mozilla/inbound/browser/app/nsBrowserApp.cpp:174
#54 0x0000000000402f83 in main (argc=5, argv=0x7fffffffd938) at /home/benjamin/dev/repos/mozilla/inbound/browser/app/nsBrowserApp.cpp:279
OK.  That stack is sane in terms of who calls what, but where is the relevant auto-no-gc?
(In reply to Boris Zbarsky (:bz) from comment #10)
> OK.  That stack is sane in terms of who calls what, but where is the
> relevant auto-no-gc?

It is probably an IntermediateNoGC created through implicit (incorrect) access to a Return<T>.  At the moment, that means look for script()-> or script().get().
Oh, OK.  Well, _that_ is right there in FunctionToString:

646     if (haveSource && !fun->script()->scriptSource()->hasSourceData() &&
647         !fun->script()->loadSource(cx, &haveSource))
Assignee: terrence → benjamin
Benjamin, thanks for taking this!

On one hand, I feel bad about adding such an annoying assertion, but on the other, each of these would have been a sec-crit bug once we go to a moving GC that we wouldn't have known about otherwise.
Attachment #674807 - Flags: review?(terrence)
(In reply to Terrence Cole [:terrence] from comment #13)
> Benjamin, thanks for taking this!
> 
> On one hand, I feel bad about adding such an annoying assertion, but on the
> other, each of these would have been a sec-crit bug once we go to a moving
> GC that we wouldn't have known about otherwise.

+1 for annoying assertions that anticipate bugs.
Comment on attachment 674807 [details] [diff] [review]
make loadSource gc safe

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

Perfect!
Attachment #674807 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/0689e804a40e

Should this have a test?
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Peter already wrote one, so Terrence or somebody should probably review it and land it.
(In reply to Ryan VanderMeulen from comment #18)
> https://hg.mozilla.org/mozilla-central/rev/0689e804a40e
> 
> Should this have a test?

It was exposed by other toSource tests, too, as comment #8 notes.
Flags: in-testsuite? → in-testsuite+
Do we have any idea why this didn't appear on tinderbox? It's super annoying to have tests fail locally when they pass on tinderbox.
I think the GC assertions are not enabled on tinderbox?
(In reply to Benjamin Peterson [:benjamin] from comment #22)
> I think the GC assertions are not enabled on tinderbox?

That's...really lame. Why aren't they? If they crash on local builds they should crash on tinderbox.
They are enabled in all debug builds. I don't know why they weren't firing in this case. Could there be a compiler issue causing problems here? Tinderbox builds are opt builds with DEBUG set. I wonder if that has something to do with it.
There is no C++ spec requirement on ordering for top-level expressions that occur between |;|'s.  This obviously causes problems for a moving GC.  This assertion would ideally catch all the spots where this /can/ happen, but for the same reason we can only catch places where it /does/ happen in practice.  The ideal solution would be for us to catch all possible problems with the compiler, but that runs into other severe problems.
You need to log in before you can comment on or make changes to this bug.