Not super pretty, and could land after we have a real nsIGlobalObject in workers. The access check isn't in a hot code path.

This needs tests, ofc.

...but perhaps better to add them once we have some real EventTarget

Comment on attachment 797494 [details] [diff] [review] patch Review of attachment 797494 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the changes made (yes I asked you to change every hunk in the patch ;-)) ::: dom/bindings/BindingUtils.cpp @@ +1733,5 @@ > GlobalObject::GetAsSupports() const > { > + if (!NS_IsMainThread()) { > + return nullptr; > + } Yeah this can just be rebased on top of my nsIGlobalObject for workers patch. ::: dom/bindings/BindingUtils.h @@ +1405,5 @@ > WantsQueryInterface<T, true> > { > static bool Enabled(JSContext* aCx, JSObject* aGlobal) > { > + return NS_IsMainThread() && IsChromeOrXBL(aCx, aGlobal); I would prefer not to add this, because we should be disabling QI on interfaces on interfaces we want on workers (really we should flip the default to be no QI and have legacy stuff opt-in). ::: dom/bindings/Codegen.py @@ +2135,5 @@ > object is the name of a JSObject* > > returns a string > """ > + return "(NS_IsMainThread() ? xpc::AccessCheck::isChrome(%s) : mozilla::dom::workers::GetWorkerPrivateFromContext(aCx)->IsChromeWorker())" % object https://bugzilla.mozilla.org/attachment.cgi?id=798945&action=diff#a/dom/bindings/BindingUtils.h_sec4 adds a function for this. @@ +8714,5 @@ > bindingHeaders['xpcprivate.h'] = webIDLFile.endswith("EventTarget.webidl") > hasWorkerStuff = len(config.getDescriptors(webIDLFile=webIDLFile, > workers=True)) != 0 > + bindingHeaders["WorkerPrivate.h"] = (hasWorkerStuff or > + any(descriptorHasChromeOnly(d) for d in descriptors)) Then you don't need this. ::: dom/workers/WorkerScope.cpp @@ +1027,5 @@ > !XMLHttpRequestUploadBinding_workers::GetConstructorObject(aCx, global) || > !URLBinding_workers::GetConstructorObject(aCx, global) || > !WorkerLocationBinding_workers::GetConstructorObject(aCx, global) || > + !WorkerNavigatorBinding_workers::GetConstructorObject(aCx, global) || > + !EventBinding::GetConstructorObject(aCx, global)) { Alphabetical order please :-)

> ::: dom/bindings/BindingUtils.h > @@ +1405,5 @@ > > WantsQueryInterface<T, true> > > { > > static bool Enabled(JSContext* aCx, JSObject* aGlobal) > > { > > + return NS_IsMainThread() && IsChromeOrXBL(aCx, aGlobal); > > I would prefer not to add this, because we should be disabling QI on > interfaces on interfaces we want on workers (really we should flip the > default to be no QI and have legacy stuff opt-in). Well, this is called not only when JS calls QI. Can't recall now when. > @@ +8714,5 @@ > > bindingHeaders['xpcprivate.h'] = webIDLFile.endswith("EventTarget.webidl") > > hasWorkerStuff = len(config.getDescriptors(webIDLFile=webIDLFile, > > workers=True)) != 0 > > + bindingHeaders["WorkerPrivate.h"] = (hasWorkerStuff or > > + any(descriptorHasChromeOnly(d) for d in descriptors)) > > Then you don't need this. I definitely needed this, but maybe I don't need anymore because of other changes :)

Steve, this introduced a bunch of rooting hazard stuff due to the NS_IsMainThread call, but I _think_ those are false positives. Is this what you were looking into the other day?

(In reply to Boris Zbarsky [:bz] from comment #9) > Steve, this introduced a bunch of rooting hazard stuff due to the > NS_IsMainThread call, but I _think_ those are false positives. Is this what > you were looking into the other day? Yes. It was saying the NS_IsMainThread could gc because it invoked a field call nsIThreadManager.GetIsMainThread. So I whitelisted that, but now it still thinks it can gc: GC Function: uint8 NS_IsMainThread() nsCOMPtr<T>::nsCOMPtr(nsGetServiceByContractID) [with T = nsIThreadManager] void nsCOMPtr_base::assign_from_gs_contractid(nsGetServiceByContractID, nsID*) uint32 nsGetServiceByContractID::operator(nsID*, void**)(const nsIID&, void**) const uint32 CallGetService(int8*, nsID*, void**) FieldCall: nsIServiceManager.GetServiceByContractID I guess I should just explicitly whitelist NS_IsMainThread. Although I thought you were also planning to root this one to shut it up?

Hmm, where should I have noticed the possible-rooting-hazard warnings?

http://people.mozilla.org/~sfink/analysis/browser/rootingHazards.txt or http://people.mozilla.org/~sfink/analysis/ :(

(In reply to Olli Pettay [:smaug] from comment #11) > Hmm, where should I have noticed the possible-rooting-hazard warnings? To be clear, it's not (yet) your responsibility to do this. But assistance is appreciated, if you can keep eye on it! We hope to have the analysis running on tbpl soon, at which time we can turn a job orange if hazards are added. Until then, someone needs to notice and look at them.

> Although I thought you were also planning to root this one to shut it up? I was hoping someone else would and I'd review. ;)

(In reply to Steve Fink [:sfink] from comment #10) > Yes. It was saying the NS_IsMainThread could gc because it invoked a field > call nsIThreadManager.GetIsMainThread. So I whitelisted that, but now it > still thinks it can gc: > > GC Function: uint8 NS_IsMainThread() > nsCOMPtr<T>::nsCOMPtr(nsGetServiceByContractID) [with T = > nsIThreadManager] > void nsCOMPtr_base::assign_from_gs_contractid(nsGetServiceByContractID, > nsID*) > uint32 nsGetServiceByContractID::operator(nsID*, void**)(const nsIID&, > void**) const > uint32 CallGetService(int8*, nsID*, void**) > FieldCall: nsIServiceManager.GetServiceByContractID > > I guess I should just explicitly whitelist NS_IsMainThread. Hmm, does your NS_IsMainThread annotation end up using the non-libxul-internal NS_IsMainThread? The commonly used NS_IsMainThread doesn't call service manager or QI. (Those would be slow.)

(In reply to Olli Pettay [:smaug] from comment #15) > Hmm, does your NS_IsMainThread annotation end up using the > non-libxul-internal NS_IsMainThread? > The commonly used NS_IsMainThread doesn't call service manager or QI. (Those > would be slow.) Well, that's a good question. The annotation just goes by function name. But the analysis seems to only be seeing http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.cpp#l117 If I read http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h#l115 correctly, then it means either MOZILLA_INTERNAL_API or NS_TLS is undefined. If I rerun the Makefile to generate the preprocessed source, it says that NS_TLS *is* defined, and MOZILLA_INTERNAL_API is not. But the latter might be because I'm just asking for the .i, and not for libxul.so? Either way, in the actual build, it doesn't seem to be defined. (Or it compiles at least one thing where it isn't defined; I'm not sure what the analysis does if it finds the same name defined twice. One probably overrides the other. Static methods are prefixed with their filename, so it's usually not a problem, but if you built two different executables, I could see getting duplicates.)