Turn off COWs for Functions

RESOLVED FIXED in mozilla36

Status

()

Core
XPConnect
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Depends on: 1 bug, {addon-compat})

unspecified
mozilla36
x86
Mac OS X
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(9 attachments)

2.30 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
4.10 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
5.34 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
1.30 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
6.44 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
13.17 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
8.72 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
1.25 KB, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
791 bytes, patch
Gabor Krizsanits (INACTIVE)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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).
(Assignee)

Updated

3 years ago
Depends on: 1082450
(Assignee)

Comment 4

3 years ago
Created attachment 8506183 [details] [diff] [review]
Part 1 - Fix up BrowserElement API to use function forwarders rather than direct chrome function references. v1
Attachment #8506183 - Flags: review?(gkrizsanits)
(Assignee)

Comment 5

3 years ago
Created attachment 8506184 [details] [diff] [review]
Part 2 - Fix up XBL tests to avoid dispatching a chrome event to content. v1

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)
(Assignee)

Comment 6

3 years ago
Created attachment 8506185 [details] [diff] [review]
Part 3 - Reindent. v1
Attachment #8506185 - Flags: review?(gkrizsanits)
(Assignee)

Comment 7

3 years ago
Created attachment 8506186 [details] [diff] [review]
Part 4 - Fix up test suite. v1
Attachment #8506186 - Flags: review?(gkrizsanits)
(Assignee)

Comment 8

3 years ago
Created attachment 8506187 [details] [diff] [review]
Part 5 - Give all non-COW filtering wrappers a null proto. v1
Attachment #8506187 - Flags: review?(gkrizsanits)
(Assignee)

Comment 9

3 years ago
Created attachment 8506188 [details] [diff] [review]
Part 6 - Generalize CheckPassToChrome machinery to operate on call/construct for all FilteringWrappers. v1
Attachment #8506188 - Flags: review?(gkrizsanits)
(Assignee)

Comment 10

3 years ago
Created attachment 8506189 [details] [diff] [review]
Part 7 - Turn off COWs for Functions. v1
Attachment #8506189 - Flags: review?(gkrizsanits)
(Assignee)

Comment 11

3 years ago
Created attachment 8506190 [details] [diff] [review]
Part 8 - Tests. v1
Attachment #8506190 - Flags: review?(gkrizsanits)
(Assignee)

Comment 12

3 years ago
Created attachment 8506817 [details] [diff] [review]
Part 0 - Fix mozPay. v1
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+
(Assignee)

Comment 17

3 years ago
(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;
(Assignee)

Updated

3 years ago
Keywords: addon-compat
(Assignee)

Updated

3 years ago
Depends on: 1086996

Updated

2 years ago
Depends on: 1163019

Updated

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