559447 - Workers: Make ChromeWorkers that have access to ctypes for browser and extensions

Status: RESOLVED FIXED

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

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

ChromeOnlyWorker is a new constructor only available in chrome pages. It allows you to create a normal worker that has access to jsctypes for interacting with outside libraries. Should be useful for extensions (weave, for instance). To use it:

  var worker = new ChromeOnlyWorker(<scriptfile.js>);

That scriptfile.js may use ctypes by simply accessing the 'ctypes' property on the global. It may also create a sub-ChromeOnlyWorker.

Content script may not see the constructor, and all ChromeOnlyWorker objects should be auto-wrapped in a SOW to keep them from being shared with content.

Includes an adaptor to run all the current jsctypes tests in a ChromeOnlyWorker and another test to make sure that web pages can't use them.

Depends on fixing bug 559442 to keep GC from crashing ;)

Comment 1

[[e-mail]]

Comment on attachment 439103 [details] [diff] [review]
Patch, v1

>diff --git a/dom/src/threads/nsDOMWorker.cpp b/dom/src/threads/nsDOMWorker.cpp

> class nsDOMWorkerFunctions
> {
> public:
>+  typedef nsDOMWorker::WorkerPrivilegeModel WorkerPrivilegeModel;
>+
>   // Same as window.dump().
>-  static JSBool Dump(JSContext* aCx, JSObject* aObj, uintN aArgc,
>-                     jsval* aArgv, jsval* aRval);
>+  static JSBool
>+  Dump(JSContext* aCx, JSObject* aObj, uintN aArgc, jsval* aArgv, jsval* aRval);

FWIW in ctypes I used a namespace for this; having a container class full of static functions felt a little bit weird.

>+JSBool
>+nsDOMWorkerFunctions::CTypesLazyGetter(JSContext* aCx,
>+                                       JSObject* aObj,
>+                                       jsval aId,
>+                                       jsval* aVp)
>+{

>+  if (worker->mPrivilegeModel != nsDOMWorker::CHROME) {
>+    NS_ERROR("This shouldn't be possible!");
>+    JS_ReportError(aCx, "Not allowed to call this!");
>+    return JS_FALSE;
>+  }

I don't really think this needs to be a release-build check, since the only way this test can fail is statically, by coder error -- the property is only added for nsDOMWorker::CHROME.

r=dwitte on the ctypes & testing bits. Very nice. :)

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1.1

Ok, shaver wanted two things:

1. s/ChromeOnlyWorker/ChromeWorker/
2. JSVERSION_LATEST for ChromeWorkers.

Both now fixed, plus I made dwitte's suggested fix about asserting rather than runtime checking privileged status in the ctypes getter.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 439122 [details] [diff] [review]
Patch, v1.1

Oh, and carrying r=dwitte

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1.2

Oops, missed one ChromeOnlyWorker...

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 439125 [details] [diff] [review]
Patch, v1.2

+#define NS_DEFINE_PRIVILEGED_CLASSINFO_DATA_WITH_NAME(_class, _name, _helper, \
+                                                      _flags)                 \

How about NS_DEFINE_CHROME_ONLY_CLASSINFO_DATA_WITH_NAME()? This isn't as much about privilege or not as it is about where we want things to show up IMO.

- In nsWindowSH::GlobalResolve():

+    // Don't expose privileged constructors to non-privileged windows.
+    if (name_struct->mPrivilegedOnly) {
+      nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager();
+      NS_ENSURE_STATE(ssm);
+
+      PRBool enabled;
+      rv = ssm->IsCapabilityEnabled("UniversalXPConnect", &enabled);
+      NS_ENSURE_SUCCESS(rv, rv);

I think this check should simply be whether the window on which this constructor is to be defined on is a chrome window or not rather than checking how we're called. If 

- In dom/base/nsDOMClassInfo.h:

+  PRBool mPrivileged;

Name that mChromeOnly IMO.

- In dom/base/nsScriptNameSpaceManager.{cpp,h}

   nsresult RegisterClassName(const char *aClassName,
                              PRInt32 aDOMClassInfoID,
+                             PRBool aPrivileged,
                              const PRUnichar **aResult);

Rename aPrivileged to aChromeOnly.

- In dom/src/threads/nsDOMWorker.h etc:

+  PRBool IsPrivileged() {
+    return mPrivilegeModel == CHROME;
+  }

IsChromeWorker()?

r=jst with that.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/mozilla-central/rev/b0a280657e23

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Apparently this broke builds that use --disable-ctypes. Duh. Followup filed as bug 560572.

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

I think this may also have broken mozilla-central xulrunner builds on win32:
  http://tinderbox.mozilla.org/showlog.cgi?log=XULRunner/1272016920.1272021822.23288.gz

Regression window:
  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5f229488969c&tochange=cdc8bf25220e

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

That seems very unlikely. My money is on bug 557060.

Comment 10

[Nick Thomas [:nthomas] (UTC+12)]

Fair enough, thanks Ben.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This was filed as bug 561408.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That's actually a dup.  Both should have been fixed by http://hg.mozilla.org/mozilla-central/rev/1475dbaf9ed5.

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Never mind, I qimported the wrong patch.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Make the sendSyncMessage use 'sync' semantics

This kind of a band-aid, dodge-the-bullet kind of patch, but I don't repro the original bug with it applied.  If anything tried to re-enter sendSyncMessage (a CPOW, say), the content process would blow up.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 441123 [details] [diff] [review]
Make the sendSyncMessage use 'sync' semantics

Yeah ... oops.

Comment 16

[Eric Shepherd [:sheppy]]

Documented here:

https://developer.mozilla.org/en/DOM/ChromeWorker

Also mentioned with links on the main Worker page, as well as the "Using web workers" page here:

https://developer.mozilla.org/En/Using_web_workers