1041626 - Xray enumeration broken for [Unforgeable] interfaces

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Bobby Holley (:bholley)]

Somehow this fell through the cracks. This basically means that enumerating Location objects over Xrays will skip all the methods.

Should be easy enough to fix. I'll write up and attach a patch.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oops.  This is totally my fault, plus lack of tests.  :(

Comment 2

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=11b6da7d94a2

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Generalize XrayEnumerateAttributes. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Mirror the logic for attribute enumeration in method enumeration. v1

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8459834 [details] [diff] [review]
Part 1 - Generalize XrayEnumerateAttributes. v1

r=me

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8459835 [details] [diff] [review]
Part 2 - Mirror the logic for attribute enumeration in method enumeration. v1

r=me

Comment 7

[Bobby Holley (:bholley)]

The above patches were what was required to get the new XOWs working, but writing a mochitest for this uncovered more broken stuff. :-(

Patches coming up.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 3 - Stop bailing out before enumerating Unforgeable attributes. v1

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 4 - Tests. v1

Comment 10

[Bobby Holley (:bholley)]

[Tracking Requested - why for this release]: Regression in FF33 with potential to break addon-compat. Fixes should be relatively low-risk and ready to land on Aurora this week.

Comment 11

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=8e4d214ddfa2

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8460395 [details] [diff] [review]
Part 3 - Stop bailing out before enumerating Unforgeable attributes. v1

Hm no this is actually wrong.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8460395 [details] [diff] [review]
Part 3 - Stop bailing out before enumerating Unforgeable attributes. v1

Won't this also enumerate the non-unforgeable bits on the object itself (i.e. when doing an OWNONLY enumeration)?  If not, why not?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8460396 [details] [diff] [review]
Part 4 - Tests. v1

>+    realOwnProperties.splice(realOwnProperties.indexOf('toJSON'), 1);

I'd rather we added "toJSON" to xrayOwnProperties instead.

And we should repeat this test with window[0].document, which has non-own bits as well as own (unforgeable) ones.

r=me with that.

Comment 15

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=b872c425804b

Comment 16

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make NativeProperties naming more consistent. v1

Comment 17

[Bobby Holley (:bholley)]

Attachment: Part 4 - Use a macro in XrayEnumerateProperties to make the logic easier to follow. v1

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 5 - Only define unforgeable attributes on instances. v1

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 6 - Do a call to XrayEnumerateNativeProps when |type| is still set to eInstance. v1

Comment 20

[Bobby Holley (:bholley)]

Attachment: Part 7 - Tests. v2 r=bz

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8460619 [details] [diff] [review]
Part 3 - Make NativeProperties naming more consistent. v1

r=me

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8460620 [details] [diff] [review]
Part 4 - Use a macro in XrayEnumerateProperties to make the logic easier to follow. v1

I'd prefer the backslashes be lined up with each other.

r=me with that.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8460621 [details] [diff] [review]
Part 5 - Only define unforgeable attributes on instances. v1

This only makes sense together with part 6, right?  Might just want to fold them together.

r=me

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8460622 [details] [diff] [review]
Part 6 - Do a call to XrayEnumerateNativeProps when |type| is still set to eInstance. v1

r=me

Comment 25

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0c79419f126a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f0a660d32231
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f5d41749f5ba
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4def803dd9f9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a5462aeda9a3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e7f5ca4f95

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0c79419f126a
https://hg.mozilla.org/mozilla-central/rev/f0a660d32231
https://hg.mozilla.org/mozilla-central/rev/f5d41749f5ba
https://hg.mozilla.org/mozilla-central/rev/4def803dd9f9
https://hg.mozilla.org/mozilla-central/rev/a5462aeda9a3
https://hg.mozilla.org/mozilla-central/rev/b2e7f5ca4f95

Comment 27

[Bobby Holley (:bholley)]

Comment on attachment 8460623 [details] [diff] [review]
Part 7 - Tests. v2 r=bz

This approval request applies to all the patches listed here.

Approval Request Comment
[Feature/regressing bug #]: bug 832014
[User impact if declined]: Small potential for addon compat issues
[Describe test coverage new/current, TBPL]: Automated test included. Landed on m-c.
[Risks and why]: Low-ish risk. This basically only affects Xrays (and thus frontend code + addons), and I think these patches strictly improve our story there. 
[String/UUID change made/needed]: None.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8460621 [details] [diff] [review]
Part 5 - Only define unforgeable attributes on instances. v1

[Triage Comment]

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8460620 [details] [diff] [review]
Part 4 - Use a macro in XrayEnumerateProperties to make the logic easier to follow. v1

[Triage Comment]

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8459835 [details] [diff] [review]
Part 2 - Mirror the logic for attribute enumeration in method enumeration. v1

[Triage Comment]

Comment 31

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8459834 [details] [diff] [review]
Part 1 - Generalize XrayEnumerateAttributes. v1

Approved because we are early in the aurora cycle.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fde6340c0cbb
https://hg.mozilla.org/releases/mozilla-aurora/rev/16c21a465ead
https://hg.mozilla.org/releases/mozilla-aurora/rev/3ff21bc2a669
https://hg.mozilla.org/releases/mozilla-aurora/rev/4bee07e2ba79
https://hg.mozilla.org/releases/mozilla-aurora/rev/e0a751a85239
https://hg.mozilla.org/releases/mozilla-aurora/rev/217d09d459e1