1065185 - Stop automatically allowed indexed and .length access on COWed arrays

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

It's time to start chipping away at the abilities of COWs. I think this might be a tractable place to start.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yes!  Let's hope we have good enough b2g test coverage....

Comment 2

[Bobby Holley (:bholley)]

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

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

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

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=b5232fa0cc11
https://tbpl.mozilla.org/?tree=Try&rev=15dde0d91cc9

Comment 7

[Bobby Holley (:bholley)]

Attachment: Fix permissions tests. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Fix crash IPC tests. v2

Comment 9

[Bobby Holley (:bholley)]

Attachment: Fix extension manager test. v1

Comment 10

[Bobby Holley (:bholley)]

Attachment: Fix call watchers. v1

See browser/devtools/canvasdebugger/test/browser_canvas-actor-test-02.js.
And note that we need to re-waive the result, otherwise we get failures in
browser/devtools/canvasdebugger/test/browser_canvas-actor-test-10.js.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Explicitly expose indexed properties in COW array test. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Turn off indexed/.length access on COW arrays. v1

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8496858 [details] [diff] [review]
Fix permissions tests. v1

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8496859 [details] [diff] [review]
Fix crash IPC tests. v2

r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8496862 [details] [diff] [review]
Fix extension manager test. v1

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8496863 [details] [diff] [review]
Fix call watchers. v1

So this is an issue for .apply only, right?  Not for .call or actual calls to functions?

This could bite some extensions...  We'll see how it goes.

r=me

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8496864 [details] [diff] [review]
Explicitly expose indexed properties in COW array test. v1

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8496865 [details] [diff] [review]
Turn off indexed/.length access on COW arrays. v1

r=yay.

Let's hope we don't have undertested b2g bits relying on this.

Comment 19

[Bobby Holley (:bholley)]

Thanks for the fast reviews!

(In reply to Boris Zbarsky [:bz] from comment #16)
> So this is an issue for .apply only, right?  Not for .call or actual calls
> to functions?

Correct.

> Let's hope we don't have undertested b2g bits relying on this.

Well, 80% of the time I've spent on this bug is on marionette-webapi, so at least the test coverage is better than it used to be. ;-)

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

Comment 20

[Bobby Holley (:bholley)]

Marionette-webapi tests are finally green. Doing a full try push:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=58eeb96d9a3e

I also can't remember whether one of the patches was actually necessary. Trying with that reverted:

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c81e1a473c52

Comment 21

[Bobby Holley (:bholley)]

Attachment: Stop using Promise.jsm over SpecialPowers in marionette tests. v1

Comment 22

[Bobby Holley (:bholley)]

Attachment: Do a better job of accessing privileged constants from voicemail marionette tests. v1

Comment 23

[Bobby Holley (:bholley)]

Attachment: Create a content Array rather than a chrome array when reading Icc contacts. v1

Right now, this array gets implicit __exposedProps__ for all of its indexed
members (and .length). Instead, we should be creating the Array directly in
the target Window.

Comment 24

[Bobby Holley (:bholley)]

Attachment: Create a content Array rather than a chrome Array in _convertWifiNetworks. v1

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8497638 [details] [diff] [review]
Do a better job of accessing privileged constants from voicemail marionette tests. v1

r=me

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8497637 [details] [diff] [review]
Stop using Promise.jsm over SpecialPowers in marionette tests. v1

I'm OK with this, obviously, as long as the people who actually own this code are... in particular with the still-not-as-good error reporting from DOM Promise.

Comment 27

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8497640 [details] [diff] [review]
Create a content Array rather than a chrome array when reading Icc contacts. v1

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

Looks good to me. Thank you.

Comment 28

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8497637 [details] [diff] [review]
Stop using Promise.jsm over SpecialPowers in marionette tests. v1

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

f+ for Telephony parts. Thank you!

f? Ben on bluetooth and f? Vincent on wifi and tethering.

Comment 29

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8497641 [details] [diff] [review]
Create a content Array rather than a chrome Array in _convertWifiNetworks. v1

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

This looks good to me, but I'd like to defer to Wifi module owner so he won't miss it :)

Comment 30

[Ben Tian (inactive)]

Comment on attachment 8497637 [details] [diff] [review]
Stop using Promise.jsm over SpecialPowers in marionette tests. v1

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

f=me for bluetooth part with nit addressed.

::: dom/bluetooth2/tests/marionette/head.js
@@ +43,5 @@
> +    this.resolve = resolve;
> +    this.reject = reject;
> +  }.bind(this));
> +  Object.freeze(this);
> +}

nit: newline here.

Comment 31

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #26)
> I'm OK with this, obviously, as long as the people who actually own this
> code are... in particular with the still-not-as-good error reporting from
> DOM Promise.

Per [1] the current setup is basically unsupported, so I don't think that objection would really stand.

[1] https://groups.google.com/d/msg/mozilla.dev.platform/rdaX9q33nGc/MprrrZcC0rEJ

Comment 32

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d958329e0289

Comment 33

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8497637 [details] [diff] [review]
Stop using Promise.jsm over SpecialPowers in marionette tests. v1

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

Vincent is taking PTO today, so I give feedback instead.
Looks good to me, thank you!

Comment 34

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8497641 [details] [diff] [review]
Create a content Array rather than a chrome Array in _convertWifiNetworks. v1

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

Vincent is taking PTO today, so I reviewed this patch instead.
Looks good, Thanks!

Comment 35

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cc213ad991a8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6871aff5d103
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/76e863b63dd8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/903fbf9bb0c7
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/981c0b7c897d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e2528549de9d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/54d6077015da
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fbdb5bead639
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7672d57acbad
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bb7b8b0da990

Comment 36

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/integration/mozilla-inbound/rev/a45d02c3b4e8 to fix the xpcshell test failure from another changeset pushed earlier today.  Bobby, can you look over that change, please?

Comment 37

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #36)
> And https://hg.mozilla.org/integration/mozilla-inbound/rev/a45d02c3b4e8 to
> fix the xpcshell test failure from another changeset pushed earlier today. 
> Bobby, can you look over that change, please?

Oh, hah. It's pretty funny that the testcase was still working 18 months later (when I pushed it this morning) and was broken by a same-day push (which I did).

This looks fine. It'll probably break again when I further declaw COWs, but this works for the time being. Thanks for doing that and averting the backout.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

This caused Gaia perma-fail as well.
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2683350&repo=mozilla-inbound

Will be backing out if I don't hear anything in short order.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Backed out per comment 38.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c14c63b4c1a

Comment 40

[Bobby Holley (:bholley)]

Attachment: Create content arrays rather than chrome arrays in DOM Apps. r=me

Yet another one of these. I think I can self-review them at this point.

Comment 41

[Bobby Holley (:bholley)]

Attachment: Expand XrayWrapper console messages for COWs. v1

I decided to invest a bit more time into making these issues detectable for
people.

Comment 42

[Bobby Holley (:bholley)]

Attachment: Turn off indexed/.length access on COW arrays. v2

Comment 43

[Bobby Holley (:bholley)]

Attachment: Console Message Tests. v1

Comment 44

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e18766f4c131

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8498945 [details] [diff] [review]
Expand XrayWrapper console messages for COWs. v1

>     // Whether we've emitted a warning about a property that was filtered out
>     // by XrayWrappers. See XrayWrapper.cpp.

Please fix this comment.

>+    // Fail silently for GET ENUMERATE, and GET_PROPERTY_DESCRIPTOR.

Comma before ENUMERATE.

>+        errorMessage.emplace("Security wrapper denied access to property %s on privileged "
...

Should this have a %s for the reason?  It doesn't seem to have one.

r=me

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8498946 [details] [diff] [review]
Turn off indexed/.length access on COW arrays. v2

r=me

Comment 47

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8498947 [details] [diff] [review]
Console Message Tests. v1

So this is testing that the message only happens on the first access, right?

r=me

Comment 48

[Bobby Holley (:bholley)]

Thanks for the fast reviews!

(In reply to Boris Zbarsky [:bz] from comment #45)
> Should this have a %s for the reason?  It doesn't seem to have one.

Whoops - I decided that the reason wasn't necessary for the console message in the COW case, but left the arg. Fixed that.


(In reply to Boris Zbarsky [:bz] from comment #47)
> So this is testing that the message only happens on the first access, right?

Right.

Comment 49

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=58012f4bb5f7

Comment 50

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8e64e5a4157c
https://hg.mozilla.org/mozilla-central/rev/0e962be2f126
https://hg.mozilla.org/mozilla-central/rev/7fe212396640
https://hg.mozilla.org/mozilla-central/rev/5f82762afba9
https://hg.mozilla.org/mozilla-central/rev/c244a7df8cfe
https://hg.mozilla.org/mozilla-central/rev/34dbc25304b9
https://hg.mozilla.org/mozilla-central/rev/2bf5d8b7d3cb
https://hg.mozilla.org/mozilla-central/rev/674d025b67e9
https://hg.mozilla.org/mozilla-central/rev/457b4fd51bb2
https://hg.mozilla.org/mozilla-central/rev/00ab79c7e6e0
https://hg.mozilla.org/mozilla-central/rev/f816a5b9c4cc
https://hg.mozilla.org/mozilla-central/rev/3bddc48c4407
https://hg.mozilla.org/mozilla-central/rev/58012f4bb5f7