757063 - document.querySelectorAll now throws when used with a proxy as argument

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alexandre Poirot [:ochameau]]

The following code was used to work in jetpack codebase before bug 703537 landing:
var str = "foo";
var proxy = Proxy.create({
  get:function (r,n) {
    // We can't return `str[n]`
    // Otherwise, it will throw following exception:
    // "String.prototype.toString called on incompatible Proxy"
    return str[n].bind(str);
  }
});
document.querySelectorAll(proxy);

It now throws following exception:
  An invalid or illegal string was specified

In order to make jetpack content scripts work again with any possible advanced JS usages (like RightJS framework), we will need a way to make querySelectorAll work again with proxies as argument.
One another alternative would be to work on bug 738244 and finally get rid of all these JS proxy code, but it would mean breaking old addons not being repacked.

Comment 1

[Bobby Holley (:bholley)]

Eddy, is this intended behavior?

Comment 2

[Eddy Bruel [:ejpbruel]]

This is probably caused by my latest patch for direct proxies. In short, ScriptedProxyHandler now inherits from IndirectProxyHandler, instead of BaseProxyHandler (the new name for ProxyHandler). Unlike ProxyHandler, IndirectProxyHandler assumes that the notion of a wrapped object, or target, exists. This allows IndirectProxyHandler to forward the Spidermonkey extension traps to the target, whereas ProxyHandler would just throw. In other words, the behavior of the SpiderMonkey extension traps for ScriptedProxyHandler changed with this patch from throwing to forwarding. In addition, ScriptedProxyHandler doesn't yet have the notion of a target, so currently the extension traps will be forwarded to the handler instead.

This is definitely not the correct behavior (for instance, calling toString on a function proxy will currently yield [Object]), but it didn't seem to cause any regressions on try. Moreover, the problem should resolve itself as soon as the ScriptedProxyHandler is updated to have a separate target and handler, which will happen in a future patch. This patch depends on the current one, though, so backing that out isn't really an option. If the problem is indeed what I think it is, a quick fix should be to overload the SpiderMonkey extension traps on the ScriptedProxyHandler to explicitly forward to BaseProxyHandler (which was the original behavior). This should allow us to proceed with the work on direct proxies for the time being without breaking any existing code.

Comment 3

[Eddy Bruel [:ejpbruel]]

Attachment: Patch to be reviewed

Looks like I was right. This patch fixes the regression as mentioned in my previous comment.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 626131 [details] [diff] [review]
Patch to be reviewed

http://zipmeme.com/uploads/generated/g1335642612204169554.jpg

Comment 5

[Eddy Bruel [:ejpbruel]]

Pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/981eaf7b3333

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/981eaf7b3333