Last Comment Bug 691707 - Enumerating new proxy-based DOM bindings should enumerate the prototype too
: Enumerating new proxy-based DOM bindings should enumerate the prototype too
Status: VERIFIED FIXED
[qa!]
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla13
Assigned To: Peter Van der Beken [:peterv]
:
:
Mentors:
: 724033 (view as bug list)
Depends on: 730484
Blocks: 648801
  Show dependency treegraph
 
Reported: 2011-10-04 03:05 PDT by Peter Van der Beken [:peterv]
Modified: 2012-04-26 04:56 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
verified
+
verified


Attachments
v1 (2.80 KB, patch)
2012-02-07 12:16 PST, Peter Van der Beken [:peterv]
mrbkap: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Peter Van der Beken [:peterv] 2011-10-04 03:05:14 PDT

    
Comment 1 Peter Van der Beken [:peterv] 2012-02-04 03:34:15 PST
*** Bug 724033 has been marked as a duplicate of this bug. ***
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-02-04 19:01:18 PST
This is apparently affecting sites in the wild.
Comment 3 Alex Keybl [:akeybl] 2012-02-06 15:07:50 PST
Tracking, but it'd be good to get more context of the sites that are affected by this bug. bug 648801 and bug 724033 didn't appear to have any pointers.
Comment 4 Simon Kornblith 2012-02-06 15:25:57 PST
In our case, it was in an extension, and we got bitten by this in two different ways.

First, we were trying to expose DOM elements from one origin to a sandbox with a different origin, using old code that iterated over an object with for(var i in obj) and created getters for the properties (from the days before JS proxy support). That got broken, but we should have switched to using proxies anyway and now we have.

Second, we created new code that used for(var i in nodeList) to iterate over the nodes in a NodeList. This is the wrong way to do it, but it worked in Firefox 10. We then realized that this code broke under Firefox 9.

The first case probably isn't a particularly big deal, since I doubt many other developers are affected. The second case is perhaps a bigger deal, since developers could end up writing code that won't work in later Firefox versions or in other browsers without noticing it.
Comment 5 Peter Van der Beken [:peterv] 2012-02-07 12:16:51 PST
Created attachment 595127 [details] [diff] [review]
v1
Comment 6 Lukas Blakk [:lsblakk] use ?needinfo 2012-02-15 16:47:04 PST
[Triage Comment]
Once landed on mozilla-central, if the patch is considered low-risk, please consider nominating for mozilla-aurora and mozilla-beta
Comment 7 Alex Keybl [:akeybl] 2012-02-23 13:57:52 PST
I've sent email to Peter about whether we should consider landing on m-c uplifting to FF11.
Comment 8 Marco Bonardo [::mak] 2012-02-24 11:08:08 PST
Backed out due to browser-chrome tests failures.
off-hand I'm not sure if it's fault of this patch or if the tests are relying on some wrong behavior that this patch fixes

it's practically all newtab tests with failures similar to:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/newtab/browser_newtab_block.js | an unexpected uncaught JS exception reported through window.onerror - this._node.addEventListener is not a function at chrome://browser/content/newtab/newTab.js:715

cc-int Tim who worked on newtab page.
Comment 9 Peter Van der Beken [:peterv] 2012-02-24 15:45:45 PST
The problem seems to be |let cells = [new Cell(this, child) for each (child in children)];| in newTab.js, with children a NodeList.
Comment 10 Peter Van der Beken [:peterv] 2012-02-24 15:51:12 PST
If I'm not mistaken that will create Cell objects for properties on the NodeList prototype, which doesn't seem what this code wants to do.
Comment 11 Tim Taubert [:ttaubert] 2012-02-24 15:59:28 PST
(In reply to Peter Van der Beken [:peterv] from comment #9)
> The problem seems to be |let cells = [new Cell(this, child) for each (child
> in children)];| in newTab.js, with children a NodeList.

Yeah, silly mistake :/ Filed bug 730484 and attached a patch.
Comment 12 Tim Taubert [:ttaubert] 2012-02-24 16:12:14 PST
Peter, there is a reviewed patch ready for checkin attached to bug 730484. Do you want to land it together with the patch from this bug? I usually land patches on fx-team, so I have no inbound repo and figured you might not want to wait until that gets merged.
Comment 13 Peter Van der Beken [:peterv] 2012-02-25 04:40:32 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/3160e7df9b4d
Comment 14 Phil Ringnalda (:philor) 2012-02-26 16:25:49 PST
https://hg.mozilla.org/mozilla-central/rev/3160e7df9b4d
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2012-02-28 09:32:35 PST
Comment on attachment 595127 [details] [diff] [review]
v1

[Approval Request Comment]
Regression caused by (bug #): 648801
User impact if declined: websites broken...
Testing completed (on m-c, etc.): landed a couple of days ago
Risk to taking this patch (and alternatives if risky): low risk
String changes made by this patch: none

I think this is something we could, and should, take on aurora. We have a duplicate already filed, and it's a very safe patch, low risk.
Comment 16 Alex Keybl [:akeybl] 2012-02-28 11:13:26 PST
Comment on attachment 595127 [details] [diff] [review]
v1

[Triage Comment]
Approving for Aurora 12. Discussed the possibility of nominating for mozilla-beta with jst, but we feel it carries too much risk for our fifth beta.
Comment 17 Peter Van der Beken [:peterv] 2012-02-28 12:30:46 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/47550f045496
Comment 18 Matt Brubeck (:mbrubeck) 2012-02-28 14:48:18 PST
Backed out on Aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/94f465b22c94

because of test failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=9700459&tree=Mozilla-Aurora

It looks like bug 730484 fixes these test failures.  If appropriate, please request approval to land bug 730484 on Aurora, so the two patches can land together.
Comment 19 Peter Van der Beken [:peterv] 2012-03-02 05:47:08 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2cd6bb9807b
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-29 15:16:35 PDT
Is this something QA can verify?
Comment 21 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-29 21:44:43 PDT
Yes.  There's a test in the patch you can verify with.
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-30 09:04:42 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #21)
> Yes.  There's a test in the patch you can verify with.

Can you give a little instruction about how to use the test in the patch?

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