1150106 - Add an async version of loadSubscript

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[[:fabrice] Fabrice Desré]

Attachment: wip

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

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: wip

same patch, just rebased.

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: wip

updated to current trunk.

Comment 3

[[:fabrice] Fabrice Desré]

Attachment: Part 1, add a `async` option to loadSubscriptWithOptions

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: Part 2, refactoring

Sharing more code in the sync and async code path.

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: Part 3, return a promise when async

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

Comment 6

[[:fabrice] Fabrice Desré]

Attachment: Part 4, xpcshell test

Comment 7

[Bobby Holley (:bholley)]

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 8

[Bobby Holley (:bholley)]

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 9

[Bobby Holley (:bholley)]

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 10

[Bobby Holley (:bholley)]

Comment on attachment 8595066 [details] [diff] [review]
Part 4, xpcshell test

Review of attachment 8595066 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 11

[Justin D'Arcangelo [:justindarc]]

Attachment: 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; }

Comment 12

[Justin D'Arcangelo [:justindarc]]

(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.

Comment 13

[Justin D'Arcangelo [:justindarc]]

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.

Comment 14

[[:fabrice] Fabrice Desré]

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.

Comment 15

[Andrew McCreight [:mccr8]]

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.

Comment 16

[Bobby Holley (:bholley)]

(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?

Comment 17

[Andrew McCreight [:mccr8]]

(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.

Comment 18

[[:fabrice] Fabrice Desré]

Attachment: async-load-addons.patch

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 19

[Andrew McCreight [:mccr8]]

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.)

Comment 20

[[:fabrice] Fabrice Desré]

(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.

Comment 21

[[:fabrice] Fabrice Desré]

Attachment: async-load-addons.patch

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

Comment 22

[Andrew McCreight [:mccr8]]

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.

Comment 23

[[:fabrice] Fabrice Desré]

Try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=6890ec57268e

And of course the debug startupcache failure doesn't repro locally...

Comment 24

[Pulsebot]

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

Comment 25

[Carsten Book [:Tomcat]]

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