Turn off COWs for Functions

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: bobbyholley, Assigned: bobbyholley)

Tracking

({addon-compat})

unspecified
mozilla36
x86
macOS
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

2.30 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
4.10 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
5.34 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.30 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
6.44 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
13.17 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
8.72 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
1.25 KB, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
791 bytes, patch
gkrizsanits
: review+
Details | Diff | Splinter Review
I think we're stuck with allowing chrome functions to be callable, but I don't think there's any good reason to allow these functions to have __exposedProps__ and use all of the other COW machinery.

One class of likely breakage here is using Function.prototype methods on COWed functions (i.e. someCowedFunction.bind). We can fix these when they come up by converting the COWed function to a forwarder (via Cu.exportFunction).
Depends on: 1082450
When I wrote these, I didn't know that simply dispatching a content event
from chrome would make it trusted.

Whitespace changes in the next patch.
Attachment #8506184 - Flags: review?(gkrizsanits)
Attachment #8506185 - Flags: review?(gkrizsanits)
Attachment #8506186 - Flags: review?(gkrizsanits)
Attachment #8506190 - Flags: review?(gkrizsanits)
Attachment #8506817 - Flags: review?(gkrizsanits)
Not sure if I can finish these reviews today, but will do it in the next few days. In general they seem reasonable, I just need to refresh my memories about a few things and double check some others.
Attachment #8506183 - Flags: review?(gkrizsanits) → review+
Attachment #8506184 - Flags: review?(gkrizsanits) → review+
Attachment #8506185 - Flags: review?(gkrizsanits) → review+
Attachment #8506186 - Flags: review?(gkrizsanits) → review+
Attachment #8506187 - Flags: review?(gkrizsanits) → review+
Comment on attachment 8506188 [details] [diff] [review]
Part 6 - Generalize CheckPassToChrome machinery to operate on call/construct for all FilteringWrappers. v1

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

::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp
@@ +132,3 @@
>      // since presumably content should never have a reason to pass an opaque
>      // object back to chrome.
> +    if (AccessCheck::isChrome(js::UncheckedUnwrap(wrapper)) && WrapperFactory::IsCOW(obj)) {

Shouldn't this extra chrome check go as well in the next patch?
Attachment #8506188 - Flags: review?(gkrizsanits) → review+
Attachment #8506189 - Flags: review?(gkrizsanits) → review+
Attachment #8506190 - Flags: review?(gkrizsanits) → review+
Attachment #8506817 - Flags: review?(gkrizsanits) → review+
(In reply to Gabor Krizsanits [:krizsa :gabor](PTO until 27th) from comment #16)
> Comment on attachment 8506188 [details] [diff] [review]
> Part 6 - Generalize CheckPassToChrome machinery to operate on call/construct
> for all FilteringWrappers. v1
> 
> Review of attachment 8506188 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/wrappers/ChromeObjectWrapper.cpp
> @@ +132,3 @@
> >      // since presumably content should never have a reason to pass an opaque
> >      // object back to chrome.
> > +    if (AccessCheck::isChrome(js::UncheckedUnwrap(wrapper)) && WrapperFactory::IsCOW(obj)) {
> 
> Shouldn't this extra chrome check go as well in the next patch?

I don't think so. There's still the issue of 2 simple Object COWs |a| and |b|, and content doing a.foo = b;
Keywords: addon-compat
Depends on: 1086996

Updated

3 years ago
Depends on: 1163019

Updated

2 years ago
Depends on: 1340914
You need to log in before you can comment on or make changes to this bug.