Closed
Bug 1005978
Opened 10 years ago
Closed 10 years ago
Add infrastructure to create main-thread WebIDL globals with XPConnect
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: peterv, Assigned: peterv)
References
Details
Attachments
(1 file)
18.13 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
I've tried to have one CreateGlobal function, controlled by a templated CreateGlobalOptions. Window needs a XPCWrappedNativeScope, which needs to be created at the right time and traced. I also had to move the code that verifies if we've traced the interface and prototype cache because we don't use TraceXPCGlobal for WebIDL globals.
Attachment #8417459 -
Flags: review?(bzbarsky)
Comment 1•10 years ago
|
||
Comment on attachment 8417459 [details] [diff] [review] v1 >+++ b/dom/bindings/BindingUtils.cpp >+CreateGlobalOptions<nsGlobalWindow>::PostCreateGlobal(JSContext* aCx, >+ JS::Handle<JSObject*> aGlobal) Fix indent? >+++ b/dom/bindings/BindingUtils.h >+ reinterpret_cast<VerifyTraceProtoAndIfaceCacheCalledTracer*>(trc)->ok = Why not static_cast? >+bool >+CreateGlobal(JSContext* aCx, T* aNative, nsWrapperCache* aCache, > NS_WARNING("Failed to create global"); > return nullptr; false? >+ dom::AllocateProtoAndIfaceCache(aGlobal, >+ CreateGlobalOptions<T>::ProtoAndIfaceCacheKind); This is changing worker globals from WindowLike to NonWindowLike, right? I guess that makes sense... >+ // Call this before doing anything that can GC, the setup of our global needs >+ // to be done before a GC happens. >+ if (!CreateGlobalOptions<T>::PostCreateGlobal(aCx, aGlobal)) { Should we wrap the stuff before this in a scope with an AutoAssertNoGC? >+++ b/js/xpconnect/src/XPCWrappedNativeScope.cpp >- MOZ_ASSERT(js::GetObjectClass(aGlobal)->flags & (JSCLASS_PRIVATE_IS_NSISUPPORTS | >- JSCLASS_HAS_PRIVATE)); Could keep asserting that either that holds or we have a DOM class. Either way. r=me
Attachment #8417459 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 2•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc9768d92d2f https://hg.mozilla.org/integration/mozilla-inbound/rev/b57bec9cf79f
Comment 3•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bc9768d92d2f https://hg.mozilla.org/mozilla-central/rev/b57bec9cf79f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Comment 4•10 years ago
|
||
(In reply to Boris Zbarsky [:bz] from comment #1) > Comment on attachment 8417459 [details] [diff] [review] > v1 > > >+ // Call this before doing anything that can GC, the setup of our global needs > >+ // to be done before a GC happens. > >+ if (!CreateGlobalOptions<T>::PostCreateGlobal(aCx, aGlobal)) { > > Should we wrap the stuff before this in a scope with an AutoAssertNoGC? > Is the intention here to suppress the analysis, or to supplement the analysis with dynamic assertions?
Flags: needinfo?(bzbarsky)
Comment 5•10 years ago
|
||
The latter, given the comments about how we can't have a GC happen before PostCreateGlobal. However, it's not clear to me that the PostCreateGlobal() should be under the scope of the AutoAssertNoGC... Peter? Also, looks like the nullptr that should have been changed to false landed as nullptr. :(
Flags: needinfo?(bzbarsky) → needinfo?(peterv)
Assignee | ||
Comment 6•10 years ago
|
||
So I think that we can actually remove the comment and the AutoAssertNoGC (since bug 1013796 landed).
Flags: needinfo?(peterv)
Comment 7•10 years ago
|
||
Thanks for the fast feedback! Try agrees that the guard is no longer needed: https://tbpl.mozilla.org/?tree=Try&rev=30be6c0e7c43 Landed the removal and the return nullptr fix as part of bug 1013531.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•