Closed Bug 571849 Opened 9 years ago Closed 9 years ago

Iterator severely broken with calls back and forth between privileged code and unprivileged code

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: protz, Assigned: mrbkap)

References

()

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

This is the followup to bug 566846. (Taking the same testcase)

Steps to reproduce:
- install the .xpi from the bug URL
- enable dump in the console
- from the Error console, run open("chrome://testcase5/content/testcase.xul",
"", "chrome,width=500,height=500");
- click "add iframe"
- click "load some content"

The three lines below:

for each (let x in Iterator(iframe.contentDocument.body.childNodes))
  dump("x is "+x+"\n");

output in the console

x is 5
x is [object XPCNativeWrapper [object Text]]
x is [object XPCNativeWrapper [object HTMLParagraphElement]]
x is [object XPCNativeWrapper [object Text]]
x is [object XPCNativeWrapper [object HTMLParagraphElement]]
x is [object XPCNativeWrapper [object Text]]
x is function XPCNativeWrapper function wrapper() {
    [native code]
}

Still better than bug 566846 but still wrong. x shouldn't be assigned 5 or the "wrapper" function.

Blake, I took the liberty of assigning to you following our discussion on IRC. I have been unable to reproduce the issue from the latest comment in bug 566846 but I'm pretty confident this is the same problem. I'm also pretty confident this is not specific to XPCNativeWrappers.

Anyway, if you want me to give your patches a try, I'm available.
Oh and by the way, Iterator used to yield an array "[index, value]". Now it only yields "value". That's a big shift from previous behavior and I think it's a bug too.
(In reply to comment #1)
Happy birthday, Andreas!
Thanks for the cc. Blake, isn't this the missing iterator-self hook for Iterator. Didn't we fix this 2 weeks ago?
I witnessed the bug with 3.7a5 which is consider recent enough. However, if I'm supposed to pull some lesser-known branch, and run tests with it, please do tell me about it so that I can stop filing useless bugs :).
FWIW I built Thunderbird 3.2 (the bug's triggered by one of my extensions) against a tracemonkey checkout and it's still there.
blocking2.0: --- → ?
Blake, sorry for insisting, but is there any chance you can take a quick look at this? You told me last time on IRC that you knew how to fix it. I'm at MozillaMessaging right now, working on some project, and this bug is hitting me all the time, so I must constantly switch back and forth between comm-1.9.2 and comm-central to test my patches, which is not convenient at all. Even a half-baked patch will do, as long as I can apply it on my own source tree!
blocking2.0: ? → beta5+
blocking2.0: beta5+ → beta6+
blocking2.0: beta7+ → betaN+
Attached patch Proposed fix v1 (obsolete) — Splinter Review
Well, the real fix here was to make JSProxyHandler::iterate properly call enumerateOwn (soon to be renamed keys in bug 600642) if JSITER_OWNONLY. However, doing so pointed out that getOwnPropertyDescriptor didn't check for own properties defined by the scriptable helper in NewResolve, so I refactored a little and came up with this. I'll push to try now, but this should be ready for review.
Attachment #489374 - Flags: review?(gal)
Attached patch Proposed fix v2Splinter Review
s/getOwnPropertyDescriptorImpl/resolveOwnProperty/
Attachment #489374 - Attachment is obsolete: true
Attachment #489564 - Flags: review?(gal)
Attachment #489374 - Flags: review?(gal)
Comment on attachment 489564 [details] [diff] [review]
Proposed fix v2

Our text coverage for xray wrappers seems to be still very weak. We keep finding bugs.
Attachment #489564 - Flags: review?(gal) → review+
http://hg.mozilla.org/tracemonkey/rev/67a69685dc56

I'll get this into m-c once tracemonkey stays green.
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/43e0c9f82545
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
This patch caused a regression. Please see this: https://bugzilla.mozilla.org/show_bug.cgi?id=611307

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101110 Firefox/4.0b8pre - Build ID: 20101111045258
Depends on: 611307
You need to log in before you can comment on or make changes to this bug.