1288770 - Switch worker timeouts to using nsJSTimeoutHandler

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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 1

[Andrea Marchesini [:baku]]

Attachment: worker_function.patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

Comment 4

[Andrea Marchesini [:baku]]

Attachment: worker_function.patch

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

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d5975fd02bd
Switch worker timeouts to using nsJSTimeoutHandler, r=smaug

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f604bac16ca
Switch worker timeouts to using nsJSTimeoutHandler, r=smaug

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0f604bac16ca

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Andrea Marchesini [:baku]]

(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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

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....

Comment 13

[Andrea Marchesini [:baku]]

Ok, follow up.