Last Comment Bug 559447 - Workers: Make ChromeWorkers that have access to ctypes for browser and extensions
: Workers: Make ChromeWorkers that have access to ctypes for browser and extens...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.3a5
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
Mentors:
Depends on: 559442
Blocks: 560572 570858
  Show dependency treegraph
 
Reported: 2010-04-14 15:08 PDT by Ben Turner (not reading bugmail, use the needinfo flag!)
Modified: 2010-06-16 10:52 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch, v1 (112.61 KB, patch)
2010-04-14 15:08 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
dwitte: review+
Details | Diff | Review
Patch, v1.1 (50.57 KB, patch)
2010-04-14 16:03 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
bent.mozilla: review+
Details | Diff | Review
Patch, v1.2 (49.66 KB, patch)
2010-04-14 16:10 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
jst: review+
Details | Diff | Review
Make the sendSyncMessage use 'sync' semantics (5.68 KB, patch)
2010-04-23 13:07 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Review

Description Ben Turner (not reading bugmail, use the needinfo flag!) 2010-04-14 15:08:37 PDT
Created attachment 439103 [details] [diff] [review]
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 dwitte@gmail.com 2010-04-14 15:31:33 PDT
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!) 2010-04-14 16:03:56 PDT
Created attachment 439122 [details] [diff] [review]
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!) 2010-04-14 16:05:53 PDT
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!) 2010-04-14 16:10:59 PDT
Created attachment 439125 [details] [diff] [review]
Patch, v1.2

Oops, missed one ChromeOnlyWorker...
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2010-04-19 16:40:21 PDT
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!) 2010-04-20 09:54:28 PDT
http://hg.mozilla.org/mozilla-central/rev/b0a280657e23
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-04-20 10:24:59 PDT
Apparently this broke builds that use --disable-ctypes. Duh. Followup filed as bug 560572.
Comment 8 Nick Thomas [:nthomas] 2010-04-23 05:25:08 PDT
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!) 2010-04-23 11:34:27 PDT
That seems very unlikely. My money is on bug 557060.
Comment 10 Nick Thomas [:nthomas] 2010-04-23 11:37:31 PDT
Fair enough, thanks Ben.
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-04-23 11:37:47 PDT
This was filed as bug 561408.
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-04-23 11:52:04 PDT
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 2010-04-23 11:54:24 PDT
Never mind, I qimported the wrong patch.
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-04-23 13:07:05 PDT
Created attachment 441123 [details] [diff] [review]
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 2010-04-23 13:08:57 PDT
Comment on attachment 441123 [details] [diff] [review]
Make the sendSyncMessage use 'sync' semantics

Yeah ... oops.
Comment 16 Eric Shepherd [:sheppy] 2010-06-16 10:52:01 PDT
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

Note You need to log in before you can comment on or make changes to this bug.