In WorkerPrivate::RunExpiredTimeouts we read stuff out of the TimeoutInfo and then pass it to JS_CallFunctionValue directly. We could fix this locally by adding relevant JS::ExposeValueToActiveJS calls. Or we could change things around to store a dom::Function instead of a JS::Value for the callable (like window does) and have Function::Call handle all this for us (as of course it does). This would require changing CC for WorkerPrivate to CC the Function but keep tracing the arguments. I vaguely prefer the latter approach, because it makes the codepath more similar to Window, though it _is_ a good bit more work.

Comment on attachment 8776007 [details] [diff] [review] worker_function.patch >+++ b/dom/base/nsJSTimeoutHandler.cpp >+ mozilla::HoldJSObjects(this); And so on. Maybe move these bits to a method shared with the window constructor? Why do you need the ErrorResult args to these constructors? They're never used.... You don't need the ErrorResult arg for the string version of NS_CreateJSTimeoutHandler you're adding either. >+++ b/dom/workers/WorkerPrivate.cpp >+WorkerPrivate::TraverseTimeouts(nsCycleCollectionTraversalCallback& aCallback) Can probably just name the argument "cb". >+WorkerPrivate::UnlinkTimeouts() Um... this is not right. It's not doing any unlinking. I'd think unlink would simply clear the mTimeouts array or something. >@@ -6173,46 +6140,54 @@ WorkerPrivate::RunExpiredTimeouts(JSCont > AutoEntryScript aes(global, reason, false); This should move into the !callback branch, no? Also means it does not need a separate scope. >+ const char16_t* script = info->mHandler->GetHandlerText(); So... this is a preexisting bug, but it's clearly wrong. We should change GetHandlerText to return a "const nsString&" or something, both here and in the window code. Consider this test: var x = 0; setTimeout("x++; '\x00'; x++"); setTimeout("console.log(x)") This should log 2, but logs a syntax error and then 0. Or this: var x = 0; setTimeout("x++; \x00; x++"); setTimeout("console.log(x)") >+ retval = true; No, retval = false, please! This should log 0, but logs 1. Please add some tests like this, worker and window? >+ JS::Rooted<JS::Value> ignoredVal(CycleCollectedJSRuntime::Get()->Runtime()); Just use aCx as the constructor argument here, please. No need to make things more complicated. ;)

I spun off bug 1291364 to just add the expose calls so we can land spidermonkey asserts. Leaving this open for the refactoring work...

smaug, bz was reviewing this patch but he is on PTO. I can wait until he is back.

Comment on attachment 8781127 [details] [diff] [review] worker_function.patch >+ // Evaluate the timeout expression. >+ nsAutoString script; >+ info->mHandler->GetHandlerText(script); >+ >+ const char* filename = nullptr; >+ uint32_t lineNo = 0, dummyColumn = 0; >+ info->mHandler->GetLocation(&filename, &lineNo, &dummyColumn); >+ >+ JS::CompileOptions options(aes.cx()); >+ options.setFileAndLine(filename, lineNo).setNoScriptRval(true); >+ >+ JS::Rooted<JS::Value> unused(aes.cx()); >+ >+ if (!JS::Evaluate(aes.cx(), options, script.get(), >+ script.Length(), &unused) && >+ !JS_IsExceptionPending(aCx)) { >+ retval = false; >+ break; > } >- else { >- nsString expression = info->mTimeoutString; >- >- JS::CompileOptions options(aCx); >- options.setFileAndLine(info->mFilename.get(), info->mLineNumber) >- .setNoScriptRval(true); >- >- JS::Rooted<JS::Value> unused(aCx); >- if (!expression.IsEmpty() && >- !JS::Evaluate(aCx, options, >- expression.get(), expression.Length(), &unused) && >- !JS_IsExceptionPending(aCx)) { >- retval = false; >- break; >- } >+ } else { >+ ErrorResult rv; >+ JS::Rooted<JS::Value> ignoredVal(aCx); >+ callback->Call(GlobalScope(), info->mHandler->GetArgs(), &ignoredVal, rv, >+ reason); >+ if (rv.IsUncatchableException()) { >+ rv.SuppressException(); >+ retval = false; >+ break; > } > } Could there be a followup patch or bug to try to add some helper method so that nsGlobalWindow and workers wouldn't have almost the same code for this stuff (but not exactly the same). > WorkerGlobalScope::WorkerGlobalScope(WorkerPrivate* aWorkerPrivate) >@@ -78,36 +91,36 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_ > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mConsole) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCrypto) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPerformance) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mLocation) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mNavigator) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mIndexedDB) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mCacheStorage) > tmp->TraverseHostObjectURIs(cb); >+ tmp->mWorkerPrivate->TraverseTimeouts(cb); > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > > NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(WorkerGlobalScope, > DOMEventTargetHelper) > tmp->mWorkerPrivate->AssertIsOnWorkerThread(); > NS_IMPL_CYCLE_COLLECTION_UNLINK(mConsole) > NS_IMPL_CYCLE_COLLECTION_UNLINK(mCrypto) > NS_IMPL_CYCLE_COLLECTION_UNLINK(mPerformance) > NS_IMPL_CYCLE_COLLECTION_UNLINK(mLocation) > NS_IMPL_CYCLE_COLLECTION_UNLINK(mNavigator) > NS_IMPL_CYCLE_COLLECTION_UNLINK(mIndexedDB) > NS_IMPL_CYCLE_COLLECTION_UNLINK(mCacheStorage) > tmp->UnlinkHostObjectURIs(); >+ tmp->mWorkerPrivate->UnlinkTimeouts(); > NS_IMPL_CYCLE_COLLECTION_UNLINK_END Hmm, timeouts should live in WorkerGlobalScope, not in WorkerPrivate, I think, but not about this bug.

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5975fd02bd Switch worker timeouts to using nsJSTimeoutHandler, r=smaug

Backed this out for asserting in ErrorResult.h when test test_errorPropagation.html runs: https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2a07ff6aefb65ba88c5ab6e5c212e4c9c695b8 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2d5975fd02bd7bcdefff2355fbd574bae80662bb Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=33919970&repo=mozilla-inbound 08:03:40 INFO - 880 INFO TEST-START | dom/workers/test/test_errorPropagation.html 08:03:40 INFO - ++DOMWINDOW == 32 (0x7f99bf3d5000) [pid = 2734] [serial = 87] [outer = 0x7f99b3558800] 08:03:40 INFO - ++DOCSHELL 0x7f99b215d800 == 9 [pid = 2734] [id = 12] 08:03:40 INFO - ++DOMWINDOW == 33 (0x7f99bf3d9c00) [pid = 2734] [serial = 88] [outer = (nil)] 08:03:40 INFO - ++DOMWINDOW == 34 (0x7f99c000fc00) [pid = 2734] [serial = 89] [outer = 0x7f99bf3d9c00] 08:03:41 INFO - Assertion failure: !Failed(), at /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ErrorResult.h:422 08:04:11 INFO - #01: mozilla::binding_danger::TErrorResult<mozilla::binding_danger::AssertAndSuppressCleanupPolicy>::~TErrorResult [dom/bindings/ErrorResult.h:141] 08:04:11 INFO - #02: mozilla::dom::workers::WorkerPrivate::RunExpiredTimeouts [dom/workers/WorkerPrivate.cpp:6179] 08:04:11 INFO - #03: mozilla::dom::workers::WorkerRunnable::Run [dom/workers/WorkerRunnable.cpp:378] 08:04:11 INFO - #04: nsTimerImpl::Fire [xpcom/threads/nsTimerImpl.cpp:525] 08:04:11 INFO - #05: nsTimerEvent::Run [mfbt/RefPtr.h:56] 08:04:11 INFO - #06: nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:1058] 08:04:11 INFO - #07: NS_ProcessNextEvent [xpcom/glue/nsThreadUtils.cpp:290] 08:04:11 INFO - #08: mozilla::dom::workers::WorkerPrivate::DoRunLoop [dom/workers/WorkerPrivate.cpp:4637] 08:04:11 INFO - #09: WorkerThreadPrimaryRunnable::Run [dom/workers/RuntimeService.cpp:2575] 08:04:11 INFO - #10: nsThread::ProcessNextEvent [xpcom/threads/nsThread.cpp:1058] 08:04:11 INFO - #11: NS_ProcessNextEvent [xpcom/glue/nsThreadUtils.cpp:290] 08:04:11 INFO - #12: mozilla::ipc::MessagePumpForNonMainThreads::Run [ipc/glue/MessagePump.cpp:339] 08:04:11 INFO - #13: MessageLoop::RunInternal [ipc/chromium/src/base/message_loop.cc:233] 08:04:11 INFO - #14: MessageLoop::Run [ipc/chromium/src/base/message_loop.cc:490] 08:04:11 INFO - #15: nsThread::ThreadFunc [xpcom/threads/nsThread.cpp:461] 08:04:11 INFO - #16: _pt_root [nsprpub/pr/src/pthreads/ptthread.c:219] 08:04:11 INFO - #17: libpthread.so.0 + 0x7e9a 08:04:11 INFO - #18: libc.so.6 + 0xf336d 08:04:11 INFO - ExceptionHandler::GenerateDump cloned child 2813 08:04:11 INFO - ExceptionHandler::SendContinueSignalToChild sent continue signal to child 08:04:11 INFO - ExceptionHandler::WaitForContinueSignal waiting for continue signal... 08:04:11 INFO - TEST-INFO | Main app process: exit 11 08:04:11 INFO - 881 INFO TEST-PASS | dom/workers/test/test_errorPropagation.html | Correct message event.message 08:04:11 INFO - 882 INFO TEST-PASS | dom/workers/test/test_errorPropagation.html | Correct message event.filename 08:04:11 INFO - 883 INFO TEST-PASS | dom/workers/test/test_errorPropagation.html | Correct message event.lineno 08:04:11 WARNING - TEST-UNEXPECTED-FAIL | dom/workers/test/test_errorPropagation.html | application terminated with exit code 11

Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0f604bac16ca Switch worker timeouts to using nsJSTimeoutHandler, r=smaug

Was there a reason to use a string outparam instead of a |const nsAString&| return value for GetHandlerText?

(In reply to Boris Zbarsky [:bz] from comment #10) > Was there a reason to use a string outparam instead of a |const nsAString&| > return value for GetHandlerText? No reasons. Usually strings are passed as outparam in our code base. I don't think we duplicate the string in this way, do we?

We do outparam when we don't know what sort of storage the callee will have, in the IDL/XPIDL cases. In cases when the callee can return a ref, there's really no reason not to. As far as copying the string, I _think_ we don't do it in this case. We do some stack allocation for the autostring that is pointless in the no-copy case, and we do some atomic refcounting that is not really needed here....

Ok, follow up.