Last Comment Bug 757063 - document.querySelectorAll now throws when used with a proxy as argument
: document.querySelectorAll now throws when used with a proxy as argument
Status: RESOLVED FIXED
[js:p1:fx16]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla15
Assigned To: Eddy Bruel [:ejpbruel]
:
Mentors:
Depends on:
Blocks: 703537 756214
  Show dependency treegraph
 
Reported: 2012-05-21 08:49 PDT by Alexandre Poirot [:ochameau]
Modified: 2012-05-23 05:21 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to be reviewed (1.96 KB, patch)
2012-05-22 12:35 PDT, Eddy Bruel [:ejpbruel]
bobbyholley: review+
Details | Diff | Splinter Review

Description Alexandre Poirot [:ochameau] 2012-05-21 08:49:42 PDT
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) (busy with Stylo) 2012-05-21 10:14:46 PDT
Eddy, is this intended behavior?
Comment 2 Eddy Bruel [:ejpbruel] 2012-05-21 19:09:46 PDT
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] 2012-05-22 12:35:50 PDT
Created attachment 626131 [details] [diff] [review]
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) (busy with Stylo) 2012-05-22 12:39:44 PDT
Comment on attachment 626131 [details] [diff] [review]
Patch to be reviewed

http://zipmeme.com/uploads/generated/g1335642612204169554.jpg
Comment 5 Eddy Bruel [:ejpbruel] 2012-05-22 12:46:00 PDT
Pushed http://hg.mozilla.org/integration/mozilla-inbound/rev/981eaf7b3333
Comment 6 Ed Morley [:emorley] 2012-05-23 05:21:21 PDT
https://hg.mozilla.org/mozilla-central/rev/981eaf7b3333

Note You need to log in before you can comment on or make changes to this bug.