Closed Bug 450449 Opened 14 years ago Closed 14 years ago

Implement 'importScripts' for worker threads

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

References

Details

Attachments

(3 files)

We need a way to load and compile external scripts on worker threads.
Attached patch Patch, v1Splinter Review
This patch implements the 'createWorkerFromURL' function on the pool as well as the 'loadScripts' function from within the worker.

  nsIDOMWorkerThread createWorkerFromURL(in AString aURL);

  void loadScripts(in AString aURL1, in AString aURL2, ...)

Current behavior is to download and compile scripts as they become available (off the main thread) but wait to execute until all scripts are compiled. Scripts are executed in the order they're passed in to the loadScripts function.
Attachment #335106 - Flags: superreview?(jst)
Attachment #335106 - Flags: review?(mrbkap)
Hm, this now depends on bug 451729.
Depends on: 451729
Talked with mrbkap today and he suggested a few changes. This patch applies on top of attachment 335106 [details] [diff] [review] and changes:

1. Runtime rooting everywhere, no more using the context.
2. Suspending requests around locks
3. Removed some !!
4. Using NS_NOTREACHED instead of NS_ASSERTION(PR_FALSE)
Attachment #335669 - Flags: review?(mrbkap)
Comment on attachment 335669 [details] [diff] [review]
Additional changes, v1

Oh, and I fixed an assertion that would fire if you reload a page super quickly.
Attachment #335106 - Flags: review?(mrbkap) → review+
Comment on attachment 335669 [details] [diff] [review]
Additional changes, v1

r=mrbkap with the changes jst pointed out in person.
Attachment #335669 - Flags: review?(mrbkap) → review+
Here are the additional changes requested by mrbkap/jst. It applies on top of attachment 335669 [details] [diff] [review] and attachment 335106 [details] [diff] [review]. Changes include:

1. Using volatile PRBools when appropriate.
2. Fixed comments on the general flow of the script loading process.
3. Renamed nsAutoRootedJSObject to nsAutoJSObjectHolder.
4. Replaced an atomic set with an ordinary set.
5. Added an absolute cap on the number of threads the service will ever create in the face of multiple malicious script loads.
Attachment #336076 - Flags: superreview?(jst)
Attachment #336076 - Flags: review?(jst)
Attachment #336076 - Flags: superreview?(jst)
Attachment #336076 - Flags: superreview?(jonas)
Attachment #336076 - Flags: review?(jst)
Attachment #336076 - Flags: review?(jonas)
Comment on attachment 336076 [details] [diff] [review]
Additional changes, v2

Doh, jst is out today.
Attachment #335106 - Flags: superreview?(jst) → superreview?(jonas)
Attachment #336076 - Flags: review?(jonas) → review?(mrbkap)
Attachment #336076 - Flags: review?(mrbkap)
Comment on attachment 336076 [details] [diff] [review]
Additional changes, v2

As I understand it, the 'volatile' declaration only need to be on the original boolean.

Smaug pointed out on IRC that we should push a sane context onto the main thread's context stack when calling the error handler, and he's right. I think that because we cancel the threads on page shift, we *should* be able to just use the context associated with the window that owns the threads.

Holding off on a final + until then.
Attachment #336076 - Flags: superreview?(jonas)
Attachment #336076 - Flags: superreview+
Attachment #336076 - Flags: review+
Comment on attachment 336076 [details] [diff] [review]
Additional changes, v2

sr=jst (and r=mrbkap).
Attachment #335106 - Flags: superreview?(jonas) → superreview+
c++ -o nsDOMWorkerThread.o -c -I../../../dist/include/system_wrappers -include ../../../config/gcc_hidden.h -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_COM_OBSOLETE -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux -I../../../dom/src/base  -I. -I. -I../../../dist/include/caps -I../../../dist/include/content -I../../../dist/include/js -I../../../dist/include/layout -I../../../dist/include/necko -I../../../dist/include/pref -I../../../dist/include/string -I../../../dist/include/widget -I../../../dist/include/xpcom -I../../../dist/include/xpconnect -I../../../dist/include   -I../../../dist/include/dom -I../../../dist/include/nspr     -I../../../dist/sdk/include    -fPIC   -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -fno-strict-aliasing -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -O2   -DMOZILLA_CLIENT -include ../../../mozilla-config.h -Wp,-MD,.deps/nsDOMWorkerThread.pp nsDOMWorkerThread.cpp
nsDOMWorkerThread.cpp:245: error: expected initializer before ‘nsDOMWorkerFunctions’
make[5]: *** [nsDOMWorkerThread.o] Error 1
make[5]: Leaving directory `/home/mdew/ff/mozilla-central/dom/src/threads'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/home/mdew/ff/mozilla-central/dom/src'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/home/mdew/ff/mozilla-central/dom'
make[2]: *** [libs_tier_gecko] Error 2
Pushed changeset 56a46aed3502 to mozilla-central.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
(In reply to comment #11)
> Pushed changeset 56a46aed3502 to mozilla-central.

Er, changeset f3df01ee0118. Oops.
Depends on: 483975
Depends on: 731332
Component: DOM: Mozilla Extensions → DOM
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.