We need it to load resources from app:// add-ons since the app:// channels only implement AsyncOpen() but not Open()

same patch, just rebased.

updated to current trunk.

Sharing more code in the sync and async code path.

The promise resolves to the script result, or rejects `undefined` if something goes wrong.

Comment on attachment 8595063 [details] [diff] [review] Part 1, add a `async` option to loadSubscriptWithOptions Review of attachment 8595063 [details] [diff] [review]: ----------------------------------------------------------------- r- on account of the GC hazard, and that's too serious to r+ with comments. But when you fix it, please flag mccr8 for review on that part, and assume r=bholley on the rest. ::: dom/apps/tests/addons/script.js @@ +1,4 @@ > document.addEventListener("DOMContentLoaded", function() { > var head = document.getElementById("header"); > head.innerHTML = "Hello World!"; > +}, false); What is this change? ::: dom/apps/tests/addons/script2.js @@ +1,4 @@ > document.addEventListener("DOMContentLoaded", function() { > var head = document.getElementById("header2"); > head.innerHTML = "Customized content"; > +}, false); Same here. ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp @@ +194,5 @@ > +private: > + virtual ~AsyncScriptLoader() {} > + > + nsRefPtr<nsIChannel> mChannel; > + JSObject* mTargetObj; This is totally unsafe, right? This needs to be a JS::Heap, needs to be traced, etc. @@ +299,5 @@ > + const nsAString& charset, > + nsIIOService* serv, bool reuseGlobal, > + bool cache) > +{ > + nsCOMPtr<nsIGlobalObject> globalObject = xpc::NativeGlobal(targetObjArg); This is unused. @@ +315,5 @@ > + nullptr, // aCallbacks > + nsIRequest::LOAD_NORMAL, > + serv); > + > + if (!NS_SUCCEEDED(rv)) { NS_ENSURE_SUCCESS(rv, rv); @@ +389,5 @@ > + rv = PrepareScript(uri, cx, target_obj, uriStr, charset, > + buf.get(), len, > + reuseGlobal, > + script, function); > + if (NS_FAILED(rv)) NS_ENSURE_SUCCESS(rv, rv); @@ +546,5 @@ > RootedScript script(cx); > if (cache && !options.ignoreCache) > rv = ReadCachedScript(cache, cachePath, cx, mSystemPrincipal, &script); > + > + // If we are doing an async load trigger it and bailout. Übernit: Needs a comma, and bailout is a noun. If we are doing an async load, trigger it and bail out.

Comment on attachment 8595064 [details] [diff] [review] Part 2, refactoring Review of attachment 8595064 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but please just fold this into part 1, since I'd rather do this directly rather than duplicating the code and then unduplicating it.

Comment on attachment 8595065 [details] [diff] [review] Part 3, return a promise when async Review of attachment 8595065 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp @@ +269,5 @@ > + if (!mPromise) { > + return; > + } > + > + AutoEntryScript aes(aGlobalObject, "async loadSubScript"); This string needs to be different - something like "loadSubScript promise rejection". Actually though, it looks like all of the callers of this already have a cx. Why not just pass the cx and kill this aes? @@ +290,5 @@ > AutoEntryScript aes(globalObject, "async loadSubScript"); > JSContext* cx = aes.cx(); > > if (NS_FAILED(aStatus)) { > + RejectPromise(globalObject); Calling this on all the failure paths is super error-prone. Please make a little class like this: class MOZ_STACK_CLASS AutoPromiseResponse { public: AutoRejectPromise(JSContext* cx, Promise* aPromise, nsIGlobalObject* aGlobalObject) : mCx(cx), mPromise(aPromise), mGlobalObject(aGlobalObject) {} ~AutoRejectPromise() { if (mPromise) { mPromise->RejectPromise(cx, mGlobalObject); } ResolvePromise(HandleValue aResolveValue) { mPromise->MaybeResolve(aResolveValue); mPromise = nullptr; } private: JSContext* mCx; nsRefPtr<Promise> mPromise; nsCOMPtr<nsIGlobalObject> mGlobalObject; }; @@ +344,3 @@ > { > nsCOMPtr<nsIGlobalObject> globalObject = xpc::NativeGlobal(targetObjArg); > + ErrorResult pRv; I'd just call this "result". @@ +349,5 @@ > + if (pRv.Failed()) { > + promise = nullptr; > + } > + > + mozilla::dom::AutoEntryScript aes(globalObject, "async loadSubScript"); This can't run script, so it should be an AutoJSAPI (which, mind you, requires a fallible Init() call). @@ +351,5 @@ > + } > + > + mozilla::dom::AutoEntryScript aes(globalObject, "async loadSubScript"); > + if (ToJSValue(aes.cx(), promise, retval)) { > + // Should never happen. Huh? This should never succeed? If it shouldn't fail, store the result in a boolean, and assert that it's true.

Comment on attachment 8595066 [details] [diff] [review] Part 4, xpcshell test Review of attachment 8595066 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

This add-on throws the following error upon injection using the patches in this bug: Error sandboxing app://8bda7fdb-4340-be40-8b2a-ba61da0c118c/script.js : [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [mozIJSSubScriptLoader.loadSubScriptWithOptions]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: resource://gre/modules/UserCustomizations.jsm :: this.UserCustomizations._injectItem/< :: line 246" data: no] This seems to originate from the first line of code in the add-on: if (window.location.href.startsWith('app://system')) { return; }

(In reply to Justin D'Arcangelo [:justindarc] from comment #11) > Created attachment 8596729 [details] > addon-custom-events.zip > > This add-on throws the following error upon injection using the patches in > this bug: > > Error sandboxing app://8bda7fdb-4340-be40-8b2a-ba61da0c118c/script.js : > [Exception... "Component returned failure code: 0x80070057 > (NS_ERROR_ILLEGAL_VALUE) [mozIJSSubScriptLoader.loadSubScriptWithOptions]" > nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: > resource://gre/modules/UserCustomizations.jsm :: > this.UserCustomizations._injectItem/< :: line 246" data: no] > > This seems to originate from the first line of code in the add-on: > if (window.location.href.startsWith('app://system')) { return; } I've just discovered that this NS_ERROR_ILLEGAL_VALUE error only happens for add-ons injected that are NOT installed on the system partition. So, any add-on installed via WebIDE or through the Customizer cannot be injected with the current patches posted here without getting this error.

Thanks to Fabrice for solving my issue over IRC. To reiterate here, the `network.disable.ipc.security` pref needs to be set to `true` for injection to work properly.

Andrew, reading https://dxr.mozilla.org/mozilla-central/source/js/public/RootingAPI.h?from=PersistentRooted&case=true#1033 it's not clear to me if I need to use PersistentRooted<> here or Heap<> and what to trace. The calling pattern is: chrome js calls loadSubscriptAsync() from a jsm, passing a global which is a sandboxed window. We trigger the async resource load which is tracked nsIStreamLoaderObserver instance (AsyncScriptLoader). When the load completes in AsyncScriptLoader::OnStreamComplete the global may have disappeared if the window has been closed for instance. We then do the script compilation and resolve the promise.

You need to trace mTargetObj, so that the JS engine knows that you are holding a pointer to it, so it can keep mTargetObj alive, and update it if it gets moved. I don't really understand what you wrote about the calling pattern, but you probably want the type of mTargetObj to be PersistentRooted<JSObject>. This will keep mTargetObj alive for as long as the AsyncScriptLoader is alive.

(In reply to Andrew McCreight [:mccr8] from comment #15) > You need to trace mTargetObj, so that the JS engine knows that you are > holding a pointer to it, so it can keep mTargetObj alive, and update it if > it gets moved. I don't really understand what you wrote about the calling > pattern, but you probably want the type of mTargetObj to be > PersistentRooted<JSObject>. This will keep mTargetObj alive for as long as > the AsyncScriptLoader is alive. Wouldn't it be better to just use a Heap<JSObject> and cycle-collect AsyncScriptLoader?

(In reply to Bobby Holley (:bholley) from comment #16) > Wouldn't it be better to just use a Heap<JSObject> and cycle-collect > AsyncScriptLoader? That would be more complicated to set up, but it would also work. It would be more future proof in case something non-networky ended up owning this in the future.

That implements the approach from comment 17, using the guide at https://developer.mozilla.org/en-US/docs/Interfacing_with_the_XPCOM_cycle_collector#Handling.C2.A0_JSObjects_fields Asking for feedback since I get a shutdown crash when running xpcshell tests for now.

Comment on attachment 8607168 [details] [diff] [review] async-load-addons.patch Review of attachment 8607168 [details] [diff] [review]: ----------------------------------------------------------------- What is the crash you are getting? Out of curiosity, why did you decide to CC this instead of using PersistentRooted? Did that not work? The CC stuff looks fine to me. ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp @@ +267,5 @@ > +}; > + > +NS_IMPL_CYCLE_COLLECTION_CLASS(AsyncScriptLoader) > + > +NS_INTERFACE_MAP_BEGIN(AsyncScriptLoader) You can instead use NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION for the first two lines here. @@ +274,5 @@ > +NS_INTERFACE_MAP_END > + > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(AsyncScriptLoader) > + tmp->mTargetObj = nullptr; > + mozilla::DropJSObjects(this); You should put the DropJSObjects() call in the destructor, not unlink, despite what the article said. (I fixed the article to say that now.)

(In reply to Andrew McCreight [:mccr8] from comment #19) > Comment on attachment 8607168 [details] [diff] [review] > async-load-addons.patch > > Review of attachment 8607168 [details] [diff] [review]: > ----------------------------------------------------------------- > > What is the crash you are getting? > > Out of curiosity, why did you decide to CC this instead of using > PersistentRooted? Did that not work? I seemed to me that you and Bobby agreed in comment 17 that this was a better approach, so I just went with that. And good news, with the destructor change the crash disappeared.

Asking for r? on the cycle collection parts as bholley reviewed was fine with the other changes.

Comment on attachment 8607191 [details] [diff] [review] async-load-addons.patch Review of attachment 8607191 [details] [diff] [review]: ----------------------------------------------------------------- Well, I figured the PersistentRooted<> would be better because it would spare you the trouble of dealing with the CC stuff, but you've written it, so it is fine. This will be more future proof. r=me for the CC stuff ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp @@ +31,5 @@ > #include "mozilla/scache/StartupCacheUtils.h" > #include "mozilla/unused.h" > #include "nsContentUtils.h" > #include "nsStringGlue.h" > +#include "nsCycleCollectionParticipant.h" For completeness, you should add in #include "mozilla/HoldDropJSObjects.h" here somewhere if it isn't in the file somewhere. @@ +274,5 @@ > + NS_INTERFACE_MAP_ENTRY(nsIStreamLoaderObserver) > +NS_INTERFACE_MAP_END > + > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(AsyncScriptLoader) > + tmp->mTargetObj = nullptr; Promise is actually cycle collected, so you might as well add in NS_IMPL_CYCLE_COLLECTION_UNLINK(mPromise) here. @@ +278,5 @@ > + tmp->mTargetObj = nullptr; > +NS_IMPL_CYCLE_COLLECTION_UNLINK_END > + > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(AsyncScriptLoader) > + NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS ...and NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mPromise) here.

Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=6890ec57268e And of course the debug startupcache failure doesn't repro locally...

https://hg.mozilla.org/integration/b2g-inbound/rev/773df031e191 https://hg.mozilla.org/integration/b2g-inbound/rev/675ced80f982

https://hg.mozilla.org/mozilla-central/rev/773df031e191 https://hg.mozilla.org/mozilla-central/rev/675ced80f982