This should be doable using the machinery that was added in bug 897655. I'm not sure yet whether we should do this for all <script> tags, or just <script async> and/or <script defer>.

\o/

Fix some leftover stuff from bug 908699. AtomToPrintableString would just return NULL on an ExclusiveContext, which caused error reporting to crash in some cases.

With bug 908301, we can deadlock when a worker thread tries to wait on another worker compressing source and that other thread is paused, as the first thread itself will never pause and no progress will be made.

DOM changes (mainly nsScriptLoader) to allow <script async> scripts to be parsed off the main thread.

Morphing the title, this bug will just be about handling <script async> asynchronously.

Comment on attachment 797563 [details] [diff] [review] bug 908301 leftovers Review of attachment 797563 [details] [diff] [review]: ----------------------------------------------------------------- Man, this is getting complicated. Can you remind me why we pause workers during GC? It seems like we might be able to get away without it (although doing so might be more work than this patch). ::: js/src/jsworkers.cpp @@ +781,5 @@ > + // loop in AutoPauseWorkersForGC. > + if (cx->workerThread()) { > + state.numPaused++; > + if (state.numPaused == state.numThreads) > + state.notify(WorkerThreadState::MAIN); Can we rename MAIN and WORKER to something like CONSUMER and PRODUCER? Otherwise, this code is pretty confusing to read. @@ +788,2 @@ > while (state.compressionInProgress(this)) > state.wait(WorkerThreadState::MAIN); It seems like multiple threads now can be waiting simultaneously on MAIN. Therefore, it seems like we should be using notifyAll(MAIN) in all the places where we're using notify(MAIN) now. Alternatively, we could use more condition variables or something. @@ +790,5 @@ > > + if (cx->workerThread()) { > + state.numPaused--; > + if (state.shouldPause) > + cx->workerThread()->pause(); Is the needed for correctness, or is it just an extra maybePause sort of check?

I'm sorry this is taking so long... I'm off tomorrow, but I plan to finish this review on Friday.

I don't think this should land while Debugger can't set breakpoints in these scripts.

Comment on attachment 797564 [details] [diff] [review] patch >+ nsAutoCString mURL; Please document that this is used to keep the "filename" used by the JS engine alive across a background compilation? >+class NotifyOffThreadScriptLoadCompletedRunnable : public nsRunnable Please put this in an anonymous namespace. >+ JS::FinishOffThreadScript(NULL, rt, mToken); nullptr, please. >+nsScriptLoader::AttemptAsyncScriptParse(nsScriptLoadRequest* aRequest) >+ if (!aRequest->mElement->GetScriptAsync() || aRequest->mIsInline) >+ return NS_ERROR_FAILURE; Curly braces around all blocks, please, here and elsewhere. >+ if (!JS::CompileOffThread(cx, global, options, Does this require cx to stay alive until OffThreadScriptLoaderCallback happens? >+ AutoPushJSContext cx(context->GetNativeContext()); I _think_ this can GC, so unrootedGlobal is a gc hazard. :( You want to put the Rooted before this line. Same thing in nsScriptLoader::EvaluateScript. It may be worth a comment that which JSContext we use here matters, for error reporting. >+ if (aRequest->mIsInline) { Given the first line of this function, this will always test false. Is it vestigial? If so, please remove. >+ mOffThreadScriptRequest = aRequest; Why do we need this? In particular, why only allow one off-thread compilation at a time? You do need onload blocking here. Right now, the unblock happens at the end of nsScriptLoader::ProcessPendingRequests if there is nothing pending. So either that needs to not unblock if there are off-thread compilations going (but then you need to call back into that code after the off-thread thing is done or something) or the off-thread compilation needs to explicitly block/unblock onload. >+++ b/dom/base/nsJSUtils.cpp >+ *aOffThreadToken = NULL; // Mark the token as having been finished. nullptr. r=me with the above fixed. Sorry for the lag. :(

Before landing, can we give this all a round of fuzzing specifically using finishOffThreadScript from bug 912321?

Gary and Christian, do you mind fuzzing this per comment 10?

In particular, it would be valuable if the fuzzers were able to pound on the usage of offThreadCompileScript+finishOffThreadScript in a threadsafe build with --ion-parallel-compile=on (generating the input analogous to what would be passed to eval/evaluate).

Do you mind quickly describing here how these two functions should be used? Is it just that you pass code to offThreadCompileScript and then just call finishOffThreadScript without any arguments?

IIUC, offThreadCompileScript(str);var result = finishOffThreadScript(); is basically the same as var result = evaluate(str); (offThreadCompileScript doesn't accept any fancy options like evaluate, though.) The two main interesting vectors to fuzz I see are (1) feeding a bunch of stuff into the string argument to offThreadCompileScript and (2) putting a bunch of JS between the calls to offThreadCompileScript and finishOffThreadScript. Note: calling offThreadCompileScript() when there is already a compilation job pending appears to throw a runtime error.

mOffThreadScriptRequest is there so that a reference is maintained on the nsScriptLoadRequest for the off thread parse, and so that it can be identified when the off thread parse finishes. It would be nice, yeah, if this was improved so that a single script loader could have multiple off thread parses in flight at once. https://hg.mozilla.org/integration/mozilla-inbound/rev/3ca22e239a1d

I see. Mind following a followup to have the event hold references to both the script load request and the nsScriptLoader or something? Then we'd be able to have more than one of these in flight at once. Might also be worth adding telemetry to see how often we avoid going off-thread because mOffThreadScriptRequest is set.

I think we want to unblock onload _after_ we execute the script. Otherwise we'll possibly fire the onload event before the script runs.

(In reply to Boris Zbarsky [:bz] from comment #18) > I think we want to unblock onload _after_ we execute the script. Otherwise > we'll possibly fire the onload event before the script runs. OK, I added a fix for this to the patch in bug 916255.

Per comment #18 (and see https://bugzilla.mozilla.org/show_bug.cgi?id=916255) this basically breaks some assumptions about how the script tag loading works... Can we back this out and reland with the fix (and hopefully a test) we saw some serious breakage in gaia because of this change in behavior.

(In reply to James Lal [:lightsofapollo] from comment #20) > Per comment #18 (and see > https://bugzilla.mozilla.org/show_bug.cgi?id=916255) this basically breaks > some assumptions about how the script tag loading works... Can we back this > out and reland with the fix (and hopefully a test) we saw some serious > breakage in gaia because of this change in behavior. I'll try to land a fix with just the OnLoad movement later today. It's going through try right now (https://tbpl.mozilla.org/?tree=Try&rev=1784cd49c8d0 as I don't know if it caused the tp5 timeouts in bug 916255 or not. Also note that parallel script parsing can be disabled via the javascript.options.parallel_parsing config option.

Patch moving UnblockOnload to the right place: https://hg.mozilla.org/integration/mozilla-inbound/rev/26b48b6dbff4

May I understand the reason why <script defer> is not parsed off main thread? What are the concerns? Thanks.

(In reply to Ting-Yu Chou [:ting] from comment #24) > May I understand the reason why <script defer> is not parsed off main > thread? What are the concerns? Thanks. I don't think there's any fundamental reason, it just hasn't been implemented. The semantics of <script async> and <script defer> are not the same.

(In reply to Brian Hackett (:bhackett) from comment #25) > (In reply to Ting-Yu Chou [:ting] from comment #24) > > May I understand the reason why <script defer> is not parsed off main > > thread? What are the concerns? Thanks. > > I don't think there's any fundamental reason, it just hasn't been > implemented. The semantics of <script async> and <script defer> are not the > same. I took the liberty to file bug 1147048 then. I use @defer much more often than I use @async for the applications I deploy.