821726 - Provide a way to loadSubScript without using the startupcache / selectively invalidate nsIStartupCache

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[cuteangel24]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20121128204232

Steps to reproduce:

I wrote a ChatZilla script, tested it, and found a bug. Then I updated the script and restarted ChatZilla. When that didn't work, I restarted Firefox.


Actual results:

Firefox kept using the old version of the script because loadSubScript caches loaded scripts, even across restarts.


Expected results:

It should have used the current version on disk.

Comment 1

[Alice0775 White]

You should add command line option -purgecaches  to clear startupCache. ( Ex. firefox.exe  -purgecaches )

Comment 2

[Alice0775 White]

Or. prior to restart the browser, you should execute invalidateCachesOnRestart method after edit the script.
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIXULRuntime#invalidateCachesOnRestart%28%29

Comment 3

[:Gijs (he/him)]

mozIJSSubscriptLoader should provide a way of bypassing the cache, or nsIStartupCache should be scriptable and allow selective invalidation of the cache. As it is, the loader can't work around the issue by providing 'funny' paths, because the cache normalizes paths and will still look for a canonicalized cached version. This is sadfaces all about if you're trying to do quick development of any kind of script. Restarting Firefox with a CLI option is Really Hard on OS X, and really shouldn't be required for something like this.

Comment 4

[Gz0]

Looking at mozJSSubScriptLoader::LoadSubScript in /home/user/src/ubuntu/firefox/firefox-24.0+build1/js/xpconnect/loader/mozJSSubScriptLoader.cpp, it should be easy to add a test for nglayout.debug.disable_xul_cache, since this pref is already use for that kind of things. If I were accustomed to submit patches, I would create a patch right away.

Comment 5

[:Gijs (he/him)]

(In reply to Gz0 from comment #4)
> Looking at mozJSSubScriptLoader::LoadSubScript in
> /home/user/src/ubuntu/firefox/firefox-24.0+build1/js/xpconnect/loader/
> mozJSSubScriptLoader.cpp, it should be easy to add a test for
> nglayout.debug.disable_xul_cache, since this pref is already use for that
> kind of things. If I were accustomed to submit patches, I would create a
> patch right away.

Yes, but that'd be a little like throwing a nuclear bomb on the cache. I'd prefer more control, tbh. Also not too hard, having looked at it... I'll try and get a patch together tomorrow.

Comment 6

[:Gijs (he/him)]

Attachment: allow bypassing script cache when using loadSubscript,

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 826688 [details] [diff] [review]
allow bypassing script cache when using loadSubscript,

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

::: js/xpconnect/idl/mozIJSSubScriptLoader.idl
@@ +11,5 @@
>  {
>      /**
>       * This method should only be called from JS!
>       * In JS, the signature looks like:
>       * rv loadSubScript (url [, obj] [, charset]);

This needs to be updated.

@@ +25,5 @@
>       * @retval rv the value returned by the sub-script
>       */
>      [implicit_jscontext]
> +    jsval loadSubScript(in AString url, [optional] in jsval obj, [optional] in AString charset,
> +                        [optional] in boolean noCache);

This API is getting too unwieldy, and I don't think we should add any new boolean params, especially if we think they're going to be used by external consumers (making this API difficult to change).

So, let's add a new overload:

jsval loadSubScriptWithOptions(in AString url, in jsval options);


Then, in C++, define LoadSubScriptOptions as a subclass of OptionsBase, much like CreateObjectInOptions and SandboxOptions. This is very straightforward to do, and should only be a few lines of code.

Then, separate out the loadSubScript logic into a static helper that takes a LoadSubScriptOptions. You can then easily funnel the two API entry points into the common logic. loadSubScript instantiates its own LoadSubScriptOptions, and fills in the relevant values. loadSubScriptWithOptions just calls ->Parse().

This should all be very very simple to do. If it's seeming complicated, please ping me.

Comment 8

[:Gijs (he/him)]

Attachment: allow bypassing script cache when using loadSubscript,

This compiles, and links, and works. But that's about it. I suspect my wide ParseString equivalent could be better, that including xpcprivate.h here is not good either, and that there's probably a thing or two else. But this is already a lot bigger than I thought when you said this was also going to be easy... :P

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 826875 [details] [diff] [review]
allow bypassing script cache when using loadSubscript,

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

This is excellent work - the issues are only cosmetic. Morally this is an r+, but there are enough cosmetic changes that it's probably worthwhile for me to scan through this again.

Would you mind adding a quick-and-dirty smoketest for this API to js/xpconnect/tests/chrome/test_chrometoSource.xul ? No need to test the ignoreCache stuff, just modify the loadSubScript call on line 52 to use loadSubScriptWithOptions so that we have basic test coverage for this path.

::: js/xpconnect/idl/mozIJSSubScriptLoader.idl
@@ +27,5 @@
> +
> +    /**
> +     * This method should only be called from JS!
> +     * In JS, the signature looks like:
> +     * rv = loadSubScript (url, [, optionsObject])

Is there any reason for making optionsObject optional? Presumably if someone just wanted the default for everything, they could just invoke loadSubScript, and leave off all the optional params.

::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
@@ +182,5 @@
> +    RootedValue target(cx, targetArg);
> +    RootedObject passedObj(cx);
> +    if (!JS_ValueToObject(cx, target, &passedObj))
> +        return NS_ERROR_ILLEGAL_VALUE;
> +    options.target = passedObj;

Replace all this with:
options.target = targetArg.isObject() ? &targetArg.toObject() : nullptr;

@@ +188,5 @@
> +}
> +
> +
> +NS_IMETHODIMP
> +mozJSSubScriptLoader::LoadSubScriptWithOptions(const nsAString& url, const Value& val,

give |val| a more descriptive name?

@@ +198,5 @@
> +        return NS_ERROR_ILLEGAL_VALUE;
> +    xpc::LoadSubScriptOptions options(cx, optionsObject);
> +    if (!options.Parse())
> +        return NS_ERROR_INVALID_ARG;
> +    return DoLoadSubscriptWithOptions(url, options, cx, retval);

As noted, I think that options object should be mandatory here. Then, this all gets simplified to:

if (!val.isObject())
  return NS_ERROR_INVALID_ARG;
LoadSubScriptOptions options(cx, &val.toObject());
if (!options.Parse())
  return NS_ERROR_INVALID_ARG;
return DoLoadSubScriptWithOptions(url, options, cx, retval);

@@ +230,5 @@
>  
>      // We base reusingGlobal off of what the loader told us, but we may not
>      // actually be using that object.
> +    if (options.target)
> +        targetObj = options.target;

Let's just initialize RootedObject targetObj(cx, options.target);

::: js/xpconnect/loader/mozJSSubScriptLoader.h
@@ +39,5 @@
>                          JSFunction **functionp);
>  
> +    nsresult DoLoadSubscriptWithOptions(const nsAString& url,
> +                                        xpc::LoadSubScriptOptions& options,
> +                                        JSContext* cx, JS::Value* retval);

I think we should forward declare LoadSubscriptOptions, define it in mozJSSubScripLoader.cpp (instead of xpcprivate.h), and move the #include of xpcprivate.h from mozJSSubScriptLoader.h to mozJSSubScriptLoader.cpp. At that point there's probably no more need for the xpc:: namespacing either.

::: js/xpconnect/src/Sandbox.cpp
@@ +1367,5 @@
>      return true;
>  }
>  
>  /*
> + * Helper that tries to get a string property form the options object.

from. This gabor-ism appears to have made its way into lots of similar comments in this file. Mind fixing them? ;-)

::: js/xpconnect/src/xpcprivate.h
@@ +3685,5 @@
> +    LoadSubScriptOptions(JSContext *cx = xpc_GetSafeJSContext(),
> +                         JS::HandleObject options = JS::NullPtr())
> +        : OptionsBase(cx, options)
> +        , target(cx)
> +        , charset(nullptr)

Does this create an nsString such that .IsVoid() is true? I haven't checked.

@@ +3698,5 @@
> +
> +    JS::RootedObject target;
> +    nsString charset;
> +    bool ignoreCache;
> +};

Note that when this moves into the cpp file, you can |using namespace JS| and remove the JS:: namespacing.

Comment 10

[:Gijs (he/him)]

(In reply to Bobby Holley (:bholley) from comment #9)
> Comment on attachment 826875 [details] [diff] [review]
> allow bypassing script cache when using loadSubscript,
> 
> Review of attachment 826875 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is excellent work - the issues are only cosmetic. Morally this is an
> r+, but there are enough cosmetic changes that it's probably worthwhile for
> me to scan through this again.
> 
> Would you mind adding a quick-and-dirty smoketest for this API to
> js/xpconnect/tests/chrome/test_chrometoSource.xul ? No need to test the
> ignoreCache stuff, just modify the loadSubScript call on line 52 to use
> loadSubScriptWithOptions so that we have basic test coverage for this path.
> 
> ::: js/xpconnect/idl/mozIJSSubScriptLoader.idl
> @@ +27,5 @@
> > +
> > +    /**
> > +     * This method should only be called from JS!
> > +     * In JS, the signature looks like:
> > +     * rv = loadSubScript (url, [, optionsObject])
> 
> Is there any reason for making optionsObject optional? Presumably if someone
> just wanted the default for everything, they could just invoke
> loadSubScript, and leave off all the optional params.
> 
> ::: js/xpconnect/loader/mozJSSubScriptLoader.cpp
> @@ +182,5 @@
> > +    RootedValue target(cx, targetArg);
> > +    RootedObject passedObj(cx);
> > +    if (!JS_ValueToObject(cx, target, &passedObj))
> > +        return NS_ERROR_ILLEGAL_VALUE;
> > +    options.target = passedObj;
> 
> Replace all this with:
> options.target = targetArg.isObject() ? &targetArg.toObject() : nullptr;
> 
> @@ +188,5 @@
> > +}
> > +
> > +
> > +NS_IMETHODIMP
> > +mozJSSubScriptLoader::LoadSubScriptWithOptions(const nsAString& url, const Value& val,
> 
> give |val| a more descriptive name?
> 
> @@ +198,5 @@
> > +        return NS_ERROR_ILLEGAL_VALUE;
> > +    xpc::LoadSubScriptOptions options(cx, optionsObject);
> > +    if (!options.Parse())
> > +        return NS_ERROR_INVALID_ARG;
> > +    return DoLoadSubscriptWithOptions(url, options, cx, retval);
> 
> As noted, I think that options object should be mandatory here. Then, this
> all gets simplified to:
> 
> if (!val.isObject())
>   return NS_ERROR_INVALID_ARG;
> LoadSubScriptOptions options(cx, &val.toObject());
> if (!options.Parse())
>   return NS_ERROR_INVALID_ARG;
> return DoLoadSubScriptWithOptions(url, options, cx, retval);
> 

This pukes (I renamed 'val' to 'optionsVal'):

 0:15.20 /Users/gkruitbosch/dev/mozilla-central/js/xpconnect/loader/mozJSSubScriptLoader.cpp:216:26: error: no matching constructor for initialization of 'LoadSubScriptOptions'
 0:15.20     LoadSubScriptOptions options(cx, &optionsVal.toObject());
 0:15.20                          ^       ~~~~~~~~~~~~~~~~~~~~~~~~~~
 0:15.20 /Users/gkruitbosch/dev/mozilla-central/js/xpconnect/loader/mozJSSubScriptLoader.cpp:38:5: note: candidate constructor not viable: cannot convert argument of incomplete type 'JSObject *' to 'HandleObject' (aka 'Handle<JSObject *>')
 0:15.20     LoadSubScriptOptions(JSContext *cx = xpc_GetSafeJSContext(),
 0:15.20     ^
 0:15.20 /Users/gkruitbosch/dev/mozilla-central/js/xpconnect/loader/mozJSSubScriptLoader.cpp:36:23: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
 0:15.21 class MOZ_STACK_CLASS LoadSubScriptOptions : public OptionsBase {
 0:15.21                       ^
 0:15.31 1 error generated.

I added "using namespace xpc" at the top of the file, and declared the options class as follows:

class MOZ_STACK_CLASS LoadSubScriptOptions : public OptionsBase {
public:
    LoadSubScriptOptions(JSContext *cx = xpc_GetSafeJSContext(),
                         HandleObject options = NullPtr())
        : OptionsBase(cx, options)
        , target(cx)
        , charset(NullString())
        , ignoreCache(false)
    { }

    virtual bool Parse() {
      return ParseObject("target", &target) &&
             ParseString("charset", charset) &&
             ParseBoolean("ignoreCache", &ignoreCache);
    }

    RootedObject target;
    nsString charset;
    bool ignoreCache;
};

Comment 11

[:Gijs (he/him)]

Attachment: allow bypassing script cache when using loadSubscript,

I've not upated the test yet as this doesn't compile (see comment #10)

Comment 12

[Bobby Holley (:bholley)]

Oh yeah, the constructor requires a handle, I guess. That's kind of dumb, because the superclass constructor roots it immediately. Feel free to just change the signature for the OptionsBase, SandboxOptions, and CreateObjectIn constructors to accept a JSObject * instead of JS::HandleObject (and default to nullptr, instead of JS::NullPtr), which should make all this work just fine.

If you don't feel like doing that, adding a temporary Rooted (as your previous patch did) is ok too, but we might as well make this nice along the way.

Comment 13

[:Gijs (he/him)]

Attachment: allow bypassing script cache when using loadSubscript,

This adds a test, and addresses your concerns, I hope. However, I missed exactly 1 of your remarks, regarding RootedObject targetObj(cx, options.target). Apparently xpconnect cares about this reusingGlobal bool, and your suggestion means I don't know how to set it correctly anymore. So I left that part similar to the previous patch for now. If you see a way to do this with your suggestion while keeping the behaviour correct, please let me know. :-)

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 827403 [details] [diff] [review]
allow bypassing script cache when using loadSubscript,

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

righteous. r=bholley

Kudos for the persistent and careful engineering in one of Gecko's more unfriendly neighborhoods. Sorry that the reuseGlobal stuff is such a mess.

::: js/xpconnect/idl/mozIJSSubScriptLoader.idl
@@ +27,5 @@
> +
> +    /**
> +     * This method should only be called from JS!
> +     * In JS, the signature looks like:
> +     * rv = loadSubScript (url, [, optionsObject])

The [] implies optionality. Remove it?

Comment 15

[:Gijs (he/him)]

Landed with the last nit fixed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/62601b803f21

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/62601b803f21
https://hg.mozilla.org/mozilla-central/rev/36b3d3f1391b

Comment 17

[u279076]

@cuteangel24, please verify this is fixed in Firefox 28 (beta.mozilla.org).

Comment 18

[Pixievolt No. 1]

I've added documentation to MDN:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/mozIJSSubScriptLoader

(Unrelated to this bug, I also added other information from the IDL file, excluding info about precompileScript which I don't understand.)

Comment 19

[Pixievolt No. 1]

It appears ignoreCache isn't working for scripts previously loaded by loadSubScript - I modified a bootstrapped extension's subscript and changed bootstrap.js to use loadSubScriptWithOptions with ignoreCache, reloaded the extension, and verified using the Add-ons Manager's Debug button that the new version of bootstrap.js was loaded, but the subscript was still loaded from cache.

Comment 20

[:Gijs (he/him)]

(In reply to pikadudeno1+bugzilla from comment #19)
> It appears ignoreCache isn't working for scripts previously loaded by
> loadSubScript - I modified a bootstrapped extension's subscript and changed
> bootstrap.js to use loadSubScriptWithOptions with ignoreCache, reloaded the
> extension, and verified using the Add-ons Manager's Debug button that the
> new version of bootstrap.js was loaded, but the subscript was still loaded
> from cache.

Thanks for the documentation!

That problem is very odd. Can you please file a new bug (if you can, make it block this one) with more detailed steps to reproduce? (ideally a test addon with which we can easily try to follow the same steps as you)

Comment 21

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #17)
> @cuteangel24, please verify this is fixed in Firefox 28 (beta.mozilla.org).

Clearing out this long overdue request.