Closed Bug 1065185 Opened 6 years ago Closed 6 years ago

Stop automatically allowed indexed and .length access on COWed arrays

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(13 files, 1 obsolete file)

1.01 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.48 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.10 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.78 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.03 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
7.85 KB, patch
bzbarsky
: review+
hsinyi
: feedback+
ben.tian
: feedback+
chucklee
: feedback+
Details | Diff | Splinter Review
1.01 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.60 KB, patch
hsinyi
: review+
Details | Diff | Splinter Review
887 bytes, patch
chucklee
: review+
Details | Diff | Splinter Review
1.80 KB, patch
bholley
: review+
fabrice
: feedback+
Details | Diff | Splinter Review
11.39 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.15 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.74 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
It's time to start chipping away at the abilities of COWs. I think this might be a tractable place to start.
Yes!  Let's hope we have good enough b2g test coverage....
Attachment #8496858 - Flags: review?(bzbarsky)
Attachment #8496859 - Flags: review?(bzbarsky)
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.
Attachment #8496863 - Flags: review?(bzbarsky)
Comment on attachment 8496858 [details] [diff] [review]
Fix permissions tests. v1

r=me
Attachment #8496858 - Flags: review?(bzbarsky) → review+
Comment on attachment 8496859 [details] [diff] [review]
Fix crash IPC tests. v2

r=me
Attachment #8496859 - Flags: review?(bzbarsky) → review+
Comment on attachment 8496862 [details] [diff] [review]
Fix extension manager test. v1

r=me
Attachment #8496862 - Flags: review?(bzbarsky) → review+
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
Attachment #8496863 - Flags: review?(bzbarsky) → review+
Comment on attachment 8496864 [details] [diff] [review]
Explicitly expose indexed properties in COW array test. v1

r=me
Attachment #8496864 - Flags: review?(bzbarsky) → review+
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.
Attachment #8496865 - Flags: review?(bzbarsky) → review+
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
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
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.
Attachment #8497640 - Flags: review?(htsai)
Comment on attachment 8497638 [details] [diff] [review]
Do a better job of accessing privileged constants from voicemail marionette tests. v1

r=me
Attachment #8497638 - Flags: review?(bzbarsky) → review+
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.
Attachment #8497637 - Flags: review?(bzbarsky) → review+
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.
Attachment #8497640 - Flags: review?(htsai) → review+
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.
Attachment #8497637 - Flags: feedback?(vchang)
Attachment #8497637 - Flags: feedback?(htsai)
Attachment #8497637 - Flags: feedback?(btian)
Attachment #8497637 - Flags: feedback+
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 :)
Attachment #8497641 - Flags: review?(htsai) → review?(vchang)
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.
Attachment #8497637 - Flags: feedback?(btian) → feedback+
(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 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!
Attachment #8497637 - Flags: feedback?(vchang) → feedback+
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!
Attachment #8497641 - Flags: review?(vchang) → review+
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?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bobbyholley)
Backed out per comment 38.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c14c63b4c1a
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bobbyholley)
Yet another one of these. I think I can self-review them at this point.
Attachment #8498904 - Flags: review+
Attachment #8498904 - Flags: feedback?(fabrice)
I decided to invest a bit more time into making these issues detectable for
people.
Attachment #8498945 - Flags: review?(bzbarsky)
Attachment #8496865 - Attachment is obsolete: true
Attachment #8498946 - Flags: review?(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
Attachment #8498945 - Flags: review?(bzbarsky) → review+
Comment on attachment 8498946 [details] [diff] [review]
Turn off indexed/.length access on COW arrays. v2

r=me
Attachment #8498946 - Flags: review?(bzbarsky) → review+
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
Attachment #8498947 - Flags: review?(bzbarsky) → review+
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.
Attachment #8498904 - Flags: feedback?(fabrice) → feedback+
Depends on: 1077780
Depends on: 1107882
You need to log in before you can comment on or make changes to this bug.