User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101128 Firefox/4.0b8pre Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101128 Firefox/4.0b8pre I've tried creating a new chrome worker using the API available to JavaScript code modules in several ways, and all have failed. In every case, I get an error such as: Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIWorkerFactory.newChromeWorker]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: resource://chromeworkertest/chromeworkertest.jsm :: <TOP_LEVEL> :: line 5" data: no] I am attaching a very simple extension that I believe should work according to publicly available documentation, but does not, at least on my system. Reproducible: Always Steps to Reproduce: 1. Install attached chromeworkertest.xpi 2. Navigate to chrome://chromeworkertest/content/chromeworkertest.xul Actual Results: An error is thrown to the error console: Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIWorkerFactory.newChromeWorker]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: resource://chromeworkertest/chromeworkertest.jsm :: <TOP_LEVEL> :: line 5" data: no] Expected Results: Code should complete without error.

As far as I can tell from reading newChromeWorker, it needs to be called from JS that's running in a window scope, because workers are associated with a particular window. It can't be called from a JS component or JS module. Does the documentation not say that?

I got that code snippet from https://developer.mozilla.org/en/JavaScript_code_modules/Using_workers_in_JavaScript_code_modules which seems to imply it should work in a JSM. The patch that created the nsIWorkerFactory interface was in response to bug 608171, Make ChromeWorkers available to JSMs.

Ah. The tests in bug 608171 don't seem to actually use a JSM, and I don't see how the code added in that bug can work in a JSM, unless I'm really missing something.... Setting blocking b8 on the assumption that we do really want to have chrome workers in JSMs in b8.

What's that assumption based on?

The blockingb8+ flag on bug 608171.

Jason, can you take this?

Workers, whether chrome or content, shouldn't need to be associated with a window. This should work. I can take. Is bug 614782 more important?

(In reply to comment #8) > Workers, whether chrome or content, shouldn't need to be associated with a > window. This should work. > > I can take. Is bug 614782 more important? No, I think this bug is more important.

It fails because a BackstagePass isn't an nsIScriptGlobalObject. We don't really need the window, though. It's mainly used to shut down workers when their window closes or navigates; for JSMs we just don't need to do that. bent is taking this.

Maybe I should have been clearer about the nsIScriptGlobalObject thing; that's what the code really wants, and the only ones we have are windows, xbl protos, and xul protos.

This appears to me to be the only bug on the beta8 blocker list without signs of progress towards a fix. It's not clear to me why this is a beta8 blocker; it appears to be a dependency of a new feature introduced after feature freeze (bug 608171) which is itself needed for a change (bug 608156) that makes sync performance acceptable. That seems like it's probably an acceptable reason to introduce a new feature after feature freeze, but I don't see why it blocks beta 8 in particular rather than "as soon as we can possibly do this, but not blocking any particular beta". Is there a reason this should block beta8 in particular? If so, what's the chance that we'll have a fix in the tree within the next two or three days?

(In reply to comment #12) > Is there a reason this should block beta8 in particular? If so, what's the > chance that we'll have a fix in the tree within the next two or three days? I'm actively working on this, should have something Monday or Tuesday, I hope. I guess the sync guys need to weigh in on whether or not this should be beta8 or beta9.

Is sync ready to use this for b8?

Here's a first stab. I added a test that uses a JSM (with XHR in it too) and it passes.

>+ // Figure out the principal to use if we're on the main thread. Otherwise >+ // this is a sub-worker and it will have its principal set to its parent's >+ // principal by the script loader. This sounds inconsistent. So if I understand things correctly, this patch attempts to make it so that from inside a JSM doing: w = new Worker("http://evil.com/firelaserbeams.js"); creates a worker with a "http://evil.com" principal. However doing w = new Worker("chrome://component/content/doWork.js"); and from within that worker doing w = new Worker("http://evil.com/firelaserbeams.js"); creates a worker with a system principal. I think we should be consistent and always use a "http://evil.com" principal.

Comment on attachment 495310 [details] [diff] [review] Patch, v1 Minusing this based on discussions with Ben

(In reply to comment #14) > Is sync ready to use this for b8? Just FYI, Sync is not going to use ChromeWorkers in b8.

--> beta9+, then

Ok, this reworks the principals to do like you suggested.

Now all Worker and ChromeWorker objects created by chrome have the system principal.

Comment on attachment 495971 [details] [diff] [review] Patch, v2.1 > nsDOMWorker::InitializeInternal(nsIScriptGlobalObject* aOwner, >+ if (isChrome) { >+ rv = ssm->GetSystemPrincipal(getter_AddRefs(mPrincipal)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // We're being created outside of a window. Need to figure out the script >+ // that is creating us in order for us to use relative URIs later on. >+ JSStackFrame* frame = JS_GetScriptedCaller(aCx, nsnull); >+ if (frame) { >+ JSScript* script = JS_GetFrameScript(aCx, frame); >+ NS_ENSURE_STATE(script); >+ >+ const char* filename = JS_GetScriptFilename(aCx, script); >+ >+ rv = NS_NewURI(getter_AddRefs(mURI), filename); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ } This isn't right. We can be created in a window even if we isChrome is true. You should QI aOwner to a domWindow and only go down this path if the QI returns null. >@@ -1813,44 +1864,69 @@ nsDOMWorker::CompileGlobalObject(JSConte >+ // Make sure we kept the system principal. >+ nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager(); >+ NS_ASSERTION(ssm, "Should never be null!"); >+ >+ PRBool isSystem; >+ if (NS_FAILED(ssm->IsSystemPrincipal(mPrincipal, &isSystem)) || !isSystem) { Just use nsContentUtils::IsSystemPrincipal and remove one level of if-nesting. >+nsDOMWorker::SetPrincipal(nsIPrincipal* aPrincipal) >+{ >+ NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); >+ NS_ASSERTION(aPrincipal, "Null pointer!"); >+ >+ mPrincipal = aPrincipal; >+} Inline this? > nsDOMWorkerScriptLoader::RunInternal() ... > if (mForWorker) { >- NS_ASSERTION(mScriptCount == 1, "Bad state!"); >- >- nsRefPtr<nsDOMWorker> parentWorker = mWorker->GetParent(); > if (parentWorker) { >- principal = parentWorker->GetPrincipal(); >- NS_ENSURE_STATE(principal); >- > baseURI = parentWorker->GetURI(); >- NS_ENSURE_STATE(baseURI); >+ NS_ASSERTION(baseURI, "Should have been set already!"); > } > else { >- principal = parentDoc->NodePrincipal(); >- NS_ENSURE_STATE(principal); >+ // May be null. >+ baseURI = mWorker->GetURI(); > >- baseURI = parentDoc->GetDocBaseURI(); >+ // Don't leave a temporary URI hanging around. >+ mWorker->ClearURI(); I don't understand this CleaerURI business? We never use baseURI for security checks right? Since documents can set their own baseURI to an arbitrary URI. In general it scares me that we are we are confusing baseuris and uris in variable names in this code and in nsDOMWorker::InitializeInternal. It's a good way to have security problems. I don't understand why we're not getting the principal in the else-path here, it seems like a principal was always available on the worker. > // If this script loader is being used to make a new worker then we need to > // do a same-origin check. Otherwise we need to clear the load with the > // security manager. >+ rv = mForWorker ? >+ principal->CheckMayLoad(uri, PR_FALSE): >+ secMan->CheckLoadURIWithPrincipal(principal, uri, 0); >+ NS_ENSURE_SUCCESS(rv, rv); What guarantees that principal is non-null here? It looked like principal was only set in some of the paths above. >+ // Update the principal of the worker if we just loaded a worker's primary >+ // script. >+ if (mForWorker) { >+ nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager(); >+ NS_ASSERTION(ssm, "Should never be null!"); >+ >+ PRBool isResource; >+ rv = NS_URIChainHasFlags(loadInfo.finalURI, >+ nsIProtocolHandler::URI_IS_UI_RESOURCE, >+ &isResource); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsIPrincipal> principal; >+ if (isResource) { >+ rv = ssm->GetSystemPrincipal(getter_AddRefs(principal)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ else { >+ nsCOMPtr<nsIChannel> channel = do_QueryInterface(request); >+ NS_ASSERTION(channel, "This should never fail!"); >+ >+ rv = ssm->GetChannelPrincipal(channel, getter_AddRefs(principal)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ NS_ASSERTION(principal, "This should never be null!"); >+ >+ mWorker->SetPrincipal(principal); >+ mWorker->SetURI(loadInfo.finalURI); >+ } This isn't very reliable if we for example change CheckLoadURI to allow webpages to load resource URIs. Why can't we use the principal retrieved in nsDOMWorker::InitializeInternal? > nsDOMWorkerXHRProxy::InitInternal() ... >+ nsresult rv = xhrConcrete->Init(worker->GetPrincipal(), scriptContext, >+ ownerWindow, worker->GetURI()); Does worker->GetURI return the baseURI or the "real" uri here?

(In reply to comment #22) > This isn't right. We can be created in a window even if we isChrome is true. Got it. Fixed. > Just use nsContentUtils::IsSystemPrincipal and remove one level of if-nesting. Ok. > Inline this? I can't, unless I rework the debug and thread includes. > We never use baseURI for security checks right? We chatted about this, renamed the member to "mBaseURI", etc. > I don't understand why we're not getting the principal in the else-path here, > it seems like a principal was always available on the worker. The diff is confusing, but all paths get a principal. > What guarantees that principal is non-null here? It looked like principal was > only set in some of the paths above. Nah, just a confusing diff. > This isn't very reliable if we for example change CheckLoadURI to allow > webpages to load resource URIs. Why can't we use the principal retrieved in > nsDOMWorker::InitializeInternal? Hm. I've changed this so that we only alter the principal if we don't have a system principal already. > Does worker->GetURI return the baseURI or the "real" uri here? Base, as always.

Comment on attachment 498558 [details] [diff] [review] Patch, v2.2 Use nsContentUtils::IsSystemPrincipal in nsDOMWorkerScriptLoader::OnStreamComplete if you can. r=me with that.

Ok, here is the set of rules that we agreed on on irc: c = new ChromeWorker(x); is only allowed if the caller "has chrome privileges" and x is a chrome or resource URI (as retrieved using channel principal and channel flags). It always creates a chrome-worker which has chrome principals. w = new Worker(y); is only allowed when either of the following is true A) The caller "has chrome privileges" B) The caller and y are same-origin In case A, if y is a chrome or resource URI (as retrieved using channel principal and channel flags) then the worker would have chrome principals, otherwise it gets null principals. In case B, we'd use the principal of that origin. "has chrome privileges" is defined as being inside a window with chrome principals, being inside a worker with chrome principals, or being inside a JSM.

(In reply to comment #26) > new ChromeWorker(x); > is only allowed if the caller "has chrome privileges" and x is a chrome or > resource URI (as retrieved using channel principal and channel flags). The channel principal and flags of x? or of the caller? What flags would you use to ensure x is chrome: or resource:, URI_IS_UI_RESOURCE? Are we OK with moz-icon: being used as a source of workers? some future custom scheme added by an add-on? Would a scheme whitelist be a better approach? examples of URI_IS_UI_RESOURCE schemes: * Open Mashups Studio & Runtime adds ompicon:, ompres:, and omphelp: * Boox adds boox: which is like resource: for the boox profile directory * a few others...

(In reply to comment #27) > The channel principal and flags of x? or of the caller? The caller. > What flags would you use to ensure x is chrome: or resource:, > URI_IS_UI_RESOURCE? Yep. > Are we OK with moz-icon: being used as a source of workers? some future > custom scheme added by an add-on? Would a scheme whitelist be a better > approach? I'm sort of ok with this... It's weird, but the caller would have to be chrome anyway, and the result would have to be compilable javascript source, so I'm fine letting extensions deal with this. Does that seem wrong?

> new ChromeWorker(x); > is only allowed if the caller "has chrome privileges" and x is a chrome or > resource URI (as retrieved using channel principal and channel flags). To make it more clear: new ChromeWorker(x); is only allowed if the caller "has chrome privileges" and x is chrome (determined by creating a channel for x and using nsIScriptSecurityManager::GetChannelPrincipal and checking that the returned principal is the system principal) or resource URI (as determined by creating a nsIURI for x and checking the resulting nsIURIs flags for the URI_IS_UI_RESOURCE flag). And yes, I'm ok with that including some weird schemes for the reason bent mentioned. Also note that I think we in general don't want to look for the calling codes principal per se, but rather get the principal of the window from which the constructor was retrieved is such an owner is available.

So we changed our minds. We're not letting chrome code load non-chrome urls for worker scripts, period.