The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
DOM
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({dev-doc-complete})

Trunk
mozilla1.9.3a5
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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 ;)
Attachment #439103 - Flags: review?(mrbkap)
Attachment #439103 - Flags: review?(jst)
Attachment #439103 - Flags: review?(dwitte)

Comment 1

7 years ago
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. :)
Attachment #439103 - Flags: review?(dwitte) → review+
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.
Attachment #439103 - Attachment is obsolete: true
Attachment #439122 - Flags: review?(jst)
Attachment #439103 - Flags: review?(mrbkap)
Attachment #439103 - Flags: review?(jst)
Summary: Make ChromeOnlyWorkers that have access to ctypes for browser and extensions → Make ChromeWorkers that have access to ctypes for browser and extensions
OS: Mac OS X → All
Comment on attachment 439122 [details] [diff] [review]
Patch, v1.1

Oh, and carrying r=dwitte
Attachment #439122 - Flags: review+
Created attachment 439125 [details] [diff] [review]
Patch, v1.2

Oops, missed one ChromeOnlyWorker...
Attachment #439122 - Attachment is obsolete: true
Attachment #439125 - Flags: review?(jst)
Attachment #439122 - Flags: review?(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.
Attachment #439125 - Flags: review?(jst) → review+
http://hg.mozilla.org/mozilla-central/rev/b0a280657e23
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Blocks: 560572
Apparently this broke builds that use --disable-ctypes. Duh. Followup filed as bug 560572.
Summary: Make ChromeWorkers that have access to ctypes for browser and extensions → Workers: Make ChromeWorkers that have access to ctypes for browser and extensions
Keywords: dev-doc-needed

Updated

7 years ago
Target Milestone: --- → mozilla1.9.3a5
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
That seems very unlikely. My money is on bug 557060.
Fair enough, thanks Ben.
This was filed as bug 561408.
That's actually a dup.  Both should have been fixed by http://hg.mozilla.org/mozilla-central/rev/1475dbaf9ed5.
Never mind, I qimported the wrong patch.
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 on attachment 441123 [details] [diff] [review]
Make the sendSyncMessage use 'sync' semantics

Yeah ... oops.
Attachment #441123 - Attachment is obsolete: true

Updated

7 years ago
Blocks: 570858
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.