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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jonathan Protzenko [:protz]]

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.

Comment 1

[Jonathan Protzenko [:protz]]

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.

Comment 2

[Jeff Walden [:Waldo]]

(In reply to comment #1)
Happy birthday, Andreas!

Comment 3

[Andreas Gal :gal]

Thanks for the cc. Blake, isn't this the missing iterator-self hook for Iterator. Didn't we fix this 2 weeks ago?

Comment 4

[Jonathan Protzenko [:protz]]

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 :).

Comment 5

[Jonathan Protzenko [:protz]]

FWIW I built Thunderbird 3.2 (the bug's triggered by one of my extensions) against a tracemonkey checkout and it's still there.

Comment 6

[Jonathan Protzenko [:protz]]

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!

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v1

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.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix v2

s/getOwnPropertyDescriptorImpl/resolveOwnProperty/

Comment 9

[Andreas Gal :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.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/67a69685dc56

I'll get this into m-c once tracemonkey stays green.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/43e0c9f82545

Comment 12

[Gary [:streetwolf52]]

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