Closed
Bug 1081990
Opened 10 years ago
Closed 10 years ago
Turn off COWs for Functions
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: bholley, Assigned: bholley)
References
Details
(Keywords: addon-compat)
Attachments
(9 files)
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).
Assignee | ||
Comment 1•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=441dbf70119f
Assignee | ||
Comment 2•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ea181c9ea0e3
Assignee | ||
Comment 3•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8b9da3ba07bc
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8506183 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 5•10 years ago
|
||
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•10 years ago
|
||
Attachment #8506185 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8506186 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8506187 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8506188 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8506189 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8506190 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8506817 -
Flags: review?(gkrizsanits)
Assignee | ||
Comment 13•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4feedef6a3ee
Comment 14•10 years ago
|
||
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.
Assignee | ||
Comment 15•10 years ago
|
||
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3741a068ea4b
Updated•10 years ago
|
Attachment #8506183 -
Flags: review?(gkrizsanits) → review+
Updated•10 years ago
|
Attachment #8506184 -
Flags: review?(gkrizsanits) → review+
Updated•10 years ago
|
Attachment #8506185 -
Flags: review?(gkrizsanits) → review+
Updated•10 years ago
|
Attachment #8506186 -
Flags: review?(gkrizsanits) → review+
Updated•10 years ago
|
Attachment #8506187 -
Flags: review?(gkrizsanits) → review+
Comment 16•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8506189 -
Flags: review?(gkrizsanits) → review+
Updated•10 years ago
|
Attachment #8506190 -
Flags: review?(gkrizsanits) → review+
Updated•10 years ago
|
Attachment #8506817 -
Flags: review?(gkrizsanits) → review+
Assignee | ||
Comment 17•10 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 | ||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=2681f9b134c2 Thanks for the reviews!
Assignee | ||
Updated•10 years ago
|
Keywords: addon-compat
Comment 19•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fc25ace509cf https://hg.mozilla.org/mozilla-central/rev/6fe41be8cfb0 https://hg.mozilla.org/mozilla-central/rev/5d6eec691636 https://hg.mozilla.org/mozilla-central/rev/7383d7a819fd https://hg.mozilla.org/mozilla-central/rev/aa887bd167cd https://hg.mozilla.org/mozilla-central/rev/c66cc3b4f587 https://hg.mozilla.org/mozilla-central/rev/712da524ebdd https://hg.mozilla.org/mozilla-central/rev/576bab9d7f4c https://hg.mozilla.org/mozilla-central/rev/2681f9b134c2
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•