Last Comment Bug 780542 - constructor functions return undefined in web console
: constructor functions return undefined in web console
Status: VERIFIED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 17 Branch
: x86 Windows XP
: -- normal (vote)
: mozilla17
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
: Paul Silaghi, QA [:pauly]
Mentors:
Depends on:
Blocks: 771429
  Show dependency treegraph
 
Reported: 2012-08-05 19:27 PDT by Watilin
Modified: 2013-04-22 07:43 PDT (History)
12 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
verified
+
verified
+
verified


Attachments
Embedded functions "F" and "createF" (237 bytes, text/html)
2012-08-07 01:59 PDT, Watilin
no flags Details
Make construction via a function proxy with an object in the constructor slot actually construct instead of just calling. (2.91 KB, patch)
2012-08-07 10:55 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
Make construction via a function proxy with an object in the constructor slot actually construct instead of just calling. (4.12 KB, patch)
2012-08-08 08:41 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
ejpbruel: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Watilin 2012-08-05 19:27:59 PDT
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20120805030518

Steps to reproduce:

(On Nightly 17.0a1 with a fresh profile)

On a page containing this script :

function F() { this.x = 1; }

I opened the web console and typed "new F()".


Actual results:

the console returned "undefined".


Expected results:

It should have returned "({x:1})".

However, when I type "new window.F()" I get the right result. This bug seems related to https://bugzilla.mozilla.org/show_bug.cgi?id=752404 although the latter have been resolved.
Comment 1 Loic 2012-08-06 01:45:03 PDT
In the latest Nightly, if I type your instructions in the web console, I see:

--
[10:43:51.175] function F() { this.x = 1; }
[10:43:51.180] undefined
--
[10:43:56.231] new F()
[10:43:56.251] ({x:1})
Comment 2 Watilin 2012-08-07 01:50:08 PDT
Of course it works when the constructor has been declared into the console.
I figured out it also works when the constructor is used inside another function declared into the page :

function createF() { return new F(); }

When typing "createF()" in the console, I get the correct "({x:1})". It seems that the "new" operator has trouble to work wih the global context.
Comment 3 Watilin 2012-08-07 01:59:19 PDT
Created attachment 649576 [details]
Embedded functions "F" and "createF"

This page contains the declaration of functions "F" and "createF" to be used in the Web Console.
Comment 4 Loic 2012-08-07 06:16:08 PDT
I followed your testcase and I found this regression range:

mozilla-central:
good=2012-07-14
bad=2012-07-15
changelog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0602e44ac248&tochange=57abb5f70e01

Suspected bug to confirm:
Boris Zbarsky — Bug 771429. Instead of using bound functions for the functions we get off the sandbox proto, use a function proxy. That allows property gets on the functions to get through. r=bholley
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-07 10:54:14 PDT
This is totally a JS engine bug; I assumed that proxy construction actually worked correctly, but it very much doesn't.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-07 10:55:45 PDT
Created attachment 649704 [details] [diff] [review]
Make construction via a function proxy with an object in the constructor slot actually construct instead of just calling.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-07 10:56:41 PDT
Requesting tracking for the regression.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-08 08:41:56 PDT
Created attachment 650145 [details] [diff] [review]
Make construction via a function proxy with an object in the constructor slot actually construct instead of just calling.
Comment 9 Eddy Bruel [:ejpbruel] 2012-08-08 14:59:04 PDT
Comment on attachment 650145 [details] [diff] [review]
Make construction via a function proxy with an object in the constructor slot actually construct instead of just calling.

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

Looking good :-)
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-08 20:56:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a047c51aafd0

Eddy, how do you feel about taking this on branches?  Seems like we need to do this if we want to fix the web console behavior there....
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-08 20:58:05 PDT
Comment on attachment 650145 [details] [diff] [review]
Make construction via a function proxy with an object in the constructor slot actually construct instead of just calling.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 771429 
User impact if declined: Calling non-DOM constructors in the web console doesn't
   work right.
Testing completed (on m-c, etc.): Passes attached automated tests.
Risk to taking this patch (and alternatives if risky): I don't know.  Eddy is a
   better source for this.
String or UUID changes made by this patch: None.
Comment 12 Ed Morley [:emorley] 2012-08-09 04:52:17 PDT
https://hg.mozilla.org/mozilla-central/rev/a047c51aafd0
Comment 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-09 17:38:00 PDT
For what it's worth, Eddy thinks risk is low.

As far as alternatives if we decide this is too risky, I see several:

1)  Ship with this problem.
2)  Back out bug 771429 and ship with its problem instead of this one.
3)  Back out bug 771429 and fix it in some other way.  I'm not quite sure how, and it
    would almost certainly be riskier than this patch.
4)  Change the function proxy code so consumers can pick between the old and new behavior
    and only opt in sandboxes to the new behavior for now.  This is probably doable if
    really needed.  I think.
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-10 12:05:17 PDT
Comment on attachment 650145 [details] [diff] [review]
Make construction via a function proxy with an object in the constructor slot actually construct instead of just calling.

I think we're ok to not do a backout and to take this on branches.
Comment 15 Joe Drew (not getting mail) 2012-08-14 12:32:46 PDT
This was pushed to aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/4cbf6aa4f022
and then backed out for making aurora orange. https://hg.mozilla.org/releases/mozilla-aurora/rev/c2d5393d005c
Comment 16 Joe Drew (not getting mail) 2012-08-14 12:37:32 PDT
Similarly landed on beta: https://hg.mozilla.org/releases/mozilla-beta/rev/ad7dd2476dfe
and then backed out: https://hg.mozilla.org/releases/mozilla-beta/rev/aa331c131a65
Comment 17 Joe Drew (not getting mail) 2012-08-14 14:09:47 PDT
Only partially landed on aurora:  https://hg.mozilla.org/releases/mozilla-aurora/rev/9211b0e0edad and hence backed out: https://hg.mozilla.org/releases/mozilla-aurora/rev/9211b0e0edad
Comment 18 Eddy Bruel [:ejpbruel] 2012-08-14 14:32:14 PDT
Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/52956b3e7be2

Hopefully wont be backed out this time
Comment 19 Eddy Bruel [:ejpbruel] 2012-08-14 14:33:55 PDT
And again to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/bc0a3117d9b4
Comment 20 Eddy Bruel [:ejpbruel] 2012-08-14 14:37:29 PDT
D'oh
Comment 21 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-14 16:39:26 PDT
For what it's worth, it looks like the first thing that got pushed was the old, obsolete, patch.  The one that didn't have approvals...
Comment 22 Paul Silaghi, QA [:pauly] 2012-08-17 02:47:54 PDT
Able to see the issue on FF 15b4.
Verified fixed on FF 15b5 on Win XP and Win 7 using the STR in comment 0 and the testcase.
Removing verifyme for now. Please add again if you can think of any edge cases related to this that should be tested.
Comment 23 Paul Silaghi, QA [:pauly] 2012-09-25 07:08:41 PDT
Verified fixed on:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20100101 Firefox/16.0b4
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 17.0a2 (2012-09-24)

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