Closed Bug 1005978 Opened 6 years ago Closed 6 years ago

Add infrastructure to create main-thread WebIDL globals with XPConnect

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: peterv, Assigned: peterv)

References

Details

Attachments

(1 file)

Attached patch v1Splinter 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 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+
https://hg.mozilla.org/mozilla-central/rev/bc9768d92d2f
https://hg.mozilla.org/mozilla-central/rev/b57bec9cf79f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(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)
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)
So I think that we can actually remove the comment and the AutoAssertNoGC (since bug 1013796 landed).
Flags: needinfo?(peterv)
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.