Closed Bug 1237177 Opened 4 years ago Closed 4 years ago

Regression: ...bind.apply() is not a function (Components.utils.evalInSandbox)

Categories

(Core :: JavaScript Engine, defect, major)

45 Branch
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox44 --- unaffected
firefox45 + fixed
firefox46 + fixed

People

(Reporter: janekptacijarabaci, Assigned: bzbarsky)

References

Details

(Keywords: addon-compat, regression)

Attachments

(1 file)

User Agent: Opera/9.80 (Windows NT 6.1; WOW64) Presto/2.12.388 Version/12.17

Steps to reproduce:

Reported to Greasemonkey here: https://github.com/greasemonkey/greasemonkey/issues/2345

Example code:
alert.bind(window).apply(null, ["hello"]);

When I run in the sandbox (wantXrays: true) I will get the following error:
TypeError: alert.bind(...).apply is not a function

Confirmed on:
Firefox 45.0a1 Nightly (2015-12-08, Build ID 20151208030212)

Not confirmed on:
Firefox 45.0a1 Nightly (2015-12-07, Build ID 20151207030210)
= alert("Hello")

Source tarballs of the nightly snapshots are not provided...
But: https://bugzilla.mozilla.org/show_bug.cgi?id=1055472 ?

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1163019
Severity: normal → major
OS: Unspecified → All
Hardware: Unspecified → All
See Also: → 1055472
See Also: → 1163019
> Source tarballs of the nightly snapshots are not provided...

What are the actual mercurial changeset ids (from about:buildconfig) for those two builds?
Flags: needinfo?(janekptacijarabaci)
(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #1)
> What are the actual mercurial changeset ids (from about:buildconfig) for
> those two builds?

Confirmed on:
Firefox 45.0a1 Nightly (2015-12-08, Build ID 20151208030212)
Build from https://hg.mozilla.org/mozilla-central/rev/2bdd9ec79799eff3ceec0a318f5a0632d918a527

Not confirmed on:
Firefox 45.0a1 Nightly (2015-12-07, Build ID 20151207030210)
Build from https://hg.mozilla.org/mozilla-central/rev/528ea05671e9bd9ccb33d1558a20691a72c85f98
Flags: needinfo?(janekptacijarabaci)
Last good: 2015-12-07
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Built from https://hg.mozilla.org/mozilla-central/rev/528ea05671e9

First bad: 2015-12-08
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Built from https://hg.mozilla.org/mozilla-central/rev/2bdd9ec79799

Pushlog
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=528ea05671e9&tochange=2bdd9ec79799


Bug 1055472 is the suspect.
Yeah, agreed.  Various other JS changes there, but that one is the most suspicious.

Eric, could you take a look, please?

In particular, in fun_bind we used to give the result the default Function.prototype, right?  But now we'll do whatever GetPrototype returned, which might be doing weird things in this xray function case...
Flags: needinfo?(efaustbmo)
That said, if I run this in the browser scratchpad:

  content.alert.bind(content).apply(null, ["hello"])

it works fine in a non-e10s build.

I do see the problem if I do this, in browser scratchpad:

  var sb = new Cu.Sandbox(content, { wantXrays: true, sandboxPrototype: content })
  Cu.evalInSandbox("alert.bind(window).apply(null, ['hello'])", sb)

Also, if I do this:

  var sb = new Cu.Sandbox(content, { wantXrays: true, sandboxPrototype: content })
  Cu.evalInSandbox("alert(alert.bind(window).__proto__)", sb)

I see "undefined" alerted.  And if I do this:

  var sb = new Cu.Sandbox(content, { wantXrays: true, sandboxPrototype: content })
  Cu.evalInSandbox("alert(alert.__proto__)", sb)

I get null.  I wonder whether this is part of the weirdness with the sandbox mirroring crud or something like that.
For extra fun:

  var sb = new Cu.Sandbox(content, { wantXrays: true, sandboxPrototype: content })
  sb.win = content;
  Cu.evalInSandbox("alert(win.alert == alert)", sb)

alerts "false".
Oho.  So the way sandboxPrototype works is that it uses a sandboxProxyHandler proxy around the given object as the actual thing on the proto chain.  And what that does in SandboxProxyHandler::getPropertyDescriptor is to wrap things like accessors in to rebind their this, because obviously invoking bareword alert with the _sandbox_ as this will not work so hot.  This wrapping creates a proxy using sandboxCallableProxyHandler as the handler and returns it.  So in my testcase above, "alert" is a sandboxCallableProxyHandler, not a JSFunction at all.

It's not clear to me whether fun_bind should in fact use target's prototype when target is a non-JSFunction callable.  But if so, then we should set up a useful proto on it.  Or perhaps it should push through proto gets to its target?  The idea is that it should be a transparent wrapper except for call().
Ah, I see.  js::GetPrototype only invokes the proxy stuff in the lazy proto case.  So perhaps we should be setting that here?
This seems to fix things.  Of course maybe we can eliminate this proxy altogether in favor of just binding the functions....
Attachment #8704675 - Flags: review?(efaustbmo)
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8704675 [details] [diff] [review]
Make the this-rebinding callable proxy sandboxes use return the prototype of its target, not its own null prototype

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

WFM. It smells a little of "spot fix" to me, but I agree that this is minimal to get this test working.
Attachment #8704675 - Flags: review?(efaustbmo) → review+
Looks like Boris answered his own question :)
Flags: needinfo?(efaustbmo)
Oh, and since this proxy does something a bit more complicated than "force this to be the window", just binding would in fact not work the same.  Not sure whether the more complicated thing is really needed.
https://hg.mozilla.org/mozilla-central/rev/824028cb6c89
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Tracking since this is a regression. 

bz, would you like to request uplift to aurora since it's also affected and since there is a beautiful test along with the patch?
Flags: needinfo?(bzbarsky)
Comment on attachment 8704675 [details] [diff] [review]
Make the this-rebinding callable proxy sandboxes use return the prototype of its target, not its own null prototype

Yikes, I thought I had.  

Approval Request Comment
[Feature/regressing bug #]: Bug 1055472
[User impact if declined]: Some extensions (and in particular some GreaseMonkey
   scripts) won't work right.
[Describe test coverage new/current, TreeHerder]: I think we have tests for
   this now...
[Risks and why]: Fairly low risk.  Just changes the prototype of bareword
   functions gotten off a sandbox global with a window Xray on its proto chain
   to be Function.prototype.
[String/UUID change made/needed]:  None.
Flags: needinfo?(bzbarsky)
Attachment #8704675 - Flags: approval-mozilla-aurora?
Comment on attachment 8704675 [details] [diff] [review]
Make the this-rebinding callable proxy sandboxes use return the prototype of its target, not its own null prototype

Please uplift to aurora, fix for regression, with test
Attachment #8704675 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thank you for the correction.

Would be great if you could solve that problem: Bug 1163019 ("See Also")
"Some extensions (and in particular some GreaseMonkey scripts) won't work right."
= This has been true for a long time (at least as far back as Firefox 36).

AFAIK: I see that it's very similar...
That's a totally different bug.  I commented there, but it has absolutely nothing to do with this bug.
I'm sorry. I meant it that way: Looks similar from a user perspective (but is a different issue).
You need to log in before you can comment on or make changes to this bug.