New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relative @require uses wrong base URI in certain cases #1874

Closed
Ventero opened this Issue Feb 12, 2014 · 4 comments

Comments

Projects
None yet
2 participants
@Ventero
Copy link
Contributor

Ventero commented Feb 12, 2014

Steps to reproduce:

  1. Open the user script from this gist: https://gist.github.com/Ventero/c75d29c0dbab12c9a2db - it doesn't matter whether the installation of the script is finished or not, as long as the install dialog showed up. Note that the @required script contains an alert("Github").
  2. Install the following user script: http://dump.ventero.de/greasemonkey/relative_require/script/requirefail2.user.js - this script contains the same @require directive as the previous script, but since it's installed from a different location, this time http://dump.ventero.de/greasemonkey/relative_require/c78475b522fe3a8a699e31312561075c2c20c742/require.js should be downloaded - which contains an alert("Not Github").
  3. Load any website.

Expected result: alert("Not Github") is executed.
Actual result: alert("Github") is executed.

The reason for this is that a nsIURI object is passed to the memoized uriFromUrl. The memoization uses essentially uneval(arguments) as cache key, which works fine as long as the arguments are primitives. For an nsIURI, uneval(uri) however results in ({}). This then leads to an incorrect cached value being returned for the @required URL during installation of the second script.

I can see 3 options on how to fix this issue:

  1. Don't memoize uriFromUrl. I don't think this is a good idea though, as this function is called quite often (for me, it's several hundred times per minute). Almost all calls are without a second argument or with a string though.
  2. Memoize only calls with a primitive or without a second argument. For a possible patch, see Ventero@729ab67.
  3. Convert all calling sites that pass a uri to pass uri.spec instead (looks like there's only 4 such callers). But this causes unnecessary overhead, as the spec strings have to be converted back to a URI inside uriFromUrl again.

@arantius arantius added this to the 1.16 milestone Feb 13, 2014

@arantius arantius closed this in fc6ffb5 Mar 21, 2014

@arantius

This comment has been minimized.

Copy link
Collaborator

arantius commented Mar 21, 2014

Take a look at the fix I selected and let me know what you think?

@Ventero

This comment has been minimized.

Copy link
Contributor

Ventero commented Mar 21, 2014

Solving this centrally in memoize rather than my suggested fix of handling this in uriFromUrl definitely seems like the better idea. However, I'm not sure if special casing nsIURI isn't going to bite us in the future (e.g. if we add a memoized function that doesn't accept a string instead of an uri object, or we add another memoized function that might receive "complex" non-uri arguments).

So maybe we could just skip the cache completely if any argument is not uneval-able - though I have no idea how that check would look like ... something like var str = uneval(JSON.stringify(arg)); JSON.stringify(JSON.parse(str)) == str; might work, but it's definitely not pretty.

Though that seems like a lot of complexity for a function that's only used 3 times so far - so I'm definitely fine with your fix.

@arantius

This comment has been minimized.

Copy link
Collaborator

arantius commented Mar 21, 2014

Oops. I intended to segregate the cache-key-args from the passed-to-wrapped-function args and did not.

@arantius arantius reopened this Mar 21, 2014

@Ventero

This comment has been minimized.

Copy link
Contributor

Ventero commented Mar 21, 2014

Ah, that makes much more sense!

It still leaves the chance of accidentally passing a complex value through memoize, which then might return an uninteded result from the cache, though. But I guess that's just something we have to look out for when adding new memoized functions.

@arantius arantius closed this in 678cde5 Apr 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment