Closed
Bug 1306579
Opened 8 years ago
Closed 8 years ago
Remove unnecessary uses of Expose*ToActiveJS
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(1 file)
45.83 KB,
patch
|
mccr8
:
review+
|
Details | Diff | Splinter Review |
Following on from bug 1297558 and bug 1306382, we can get rid of many calls to Expose*ToActiveJS.
Assignee | ||
Comment 1•8 years ago
|
||
Patch to remove use of Expose*ToActiveJS() when reading out of a Heap<T> since this now happens automatically. I simplified some uses to call exposeToActiveJS() on the Heap<T> which conditionally does the expose if the contents are non-null. I also removed nsIScriptContext::GetWindowProxyPreserveColor since it was not used.
Assignee: nobody → jcoppeard
Attachment #8801101 -
Flags: review?(continuation)
Comment 2•8 years ago
|
||
Comment on attachment 8801101 [details] [diff] [review] bug1306579-remove-expose-calls Review of attachment 8801101 [details] [diff] [review]: ----------------------------------------------------------------- Can you make calling ExposeActiveObjectToActiveJS a compile error? That might be neat. Maybe it isn't worthwhile. It would be worth taking a look at CodeGen.py, too. I see 201 instances of ExposeObjectToActiveJS in WebIDL codegenned files. There are only three instances of it. I can fix that if you want. The code looks like the same sort of thing you are fixing here: JS::ExposeObjectToActiveJS(result); args.rval().setObject(*result); ::: dom/base/nsFrameMessageManager.cpp @@ +1904,5 @@ > void > nsMessageManagerScriptExecutor::MarkScopesForCC() > { > for (uint32_t i = 0; i < mAnonymousGlobalScopes.Length(); ++i) { > + mAnonymousGlobalScopes[i].exposeToActiveJS(); Much nicer. ::: dom/base/nsGlobalWindow.cpp @@ -2648,5 @@ > // We're reusing the inner window, but this still counts as a navigation, > // so all expandos and such defined on the outer window should go away. Force > // all Xray wrappers to be recomputed. > - JS::Rooted<JSObject*> rootedObject(cx, GetWrapperPreserveColor()); > - JS::ExposeObjectToActiveJS(rootedObject); Well, that's odd... ::: dom/promise/PromiseCallback.cpp @@ +145,1 @@ > JS::ExposeValueToActiveJS(aValue); Can you get rid of this call for aValue here and below? I'd guess that the Rooted<> value call would do an expose.
Attachment #8801101 -
Flags: review?(continuation) → review+
Comment 3•8 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #2) > Can you get rid of this call for aValue here and below? I'd guess that the > Rooted<> value call would do an expose. I don't mean you necessarily need to fix this, given that it is in old promises code we don't even compile any more; I was just curious about whether it would be ok.
Comment 4•8 years ago
|
||
I just filed bug 1310321 for the CodeGen.py part. I can fix that.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Andrew McCreight [:mccr8] from comment #2) > ::: dom/promise/PromiseCallback.cpp > @@ +145,1 @@ > > JS::ExposeValueToActiveJS(aValue); > > Can you get rid of this call for aValue here and below? I'd guess that the > Rooted<> value call would do an expose. As noted in bug 1310321 there's no read barrier on Rooted. > I just filed bug 1310321 for the CodeGen.py part. I can fix that. Cheers :)
Comment 6•8 years ago
|
||
It's not clear to me how a gray value would enter ResolvePromiseCallback::Call as aValue. I suspect that can't happen now that Heap has a read barrier. But it's also not worth worrying about too much, because all this code is #ifndef SPIDERMONKEY_PROMISE and we should instead aim to remove it or something.
Assignee | ||
Comment 7•8 years ago
|
||
Ugh, I pushed this with the wrong bug number (bug 1297558): https://hg.mozilla.org/integration/mozilla-inbound/rev/1538850bba0f https://hg.mozilla.org/mozilla-central/rev/1538850bba0f
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Updated•8 years ago
|
status-firefox52:
--- → fixed
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•