Closed Bug 1081990 Opened 10 years ago Closed 10 years ago

Turn off COWs for Functions

Categories

(Core :: XPConnect, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: bholley, Assigned: bholley)

References

Details

(Keywords: addon-compat)

Attachments

(9 files)

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 #8506189 - 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
Depends on: 1163019
Depends on: 1340914
You need to log in before you can comment on or make changes to this bug.