720808 - Add nsJSEventListener and nsGlobalWindow to BBP

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 1

[Andrew McCreight [:mccr8]]

Comment on attachment 591230 [details] [diff] [review]
patch

Review of attachment 591230 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsCCUncollectableMarker.h
@@ +59,4 @@
>    static bool InGeneration(nsCycleCollectionTraversalCallback &cb,
> +                           PRUint32 aGeneration)
> +  {
> +    return !cb.WantAllTraces() && InGeneration(aGeneration);

This is kind of a silly thing, but it would probably be better to put WantAllTraces second, because "!cb.WantAllTraces()" is always true in cases where we care about speed, so we should do the check that can fail first.  Not a huge deal, I'm sure, but this particular function likely gets called a lot.

::: dom/base/nsGlobalWindow.cpp
@@ +1418,5 @@
> +  }
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_END
> +
> +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(nsGlobalWindow)
> +  return tmp->IsBlackForCC();

This is totally fine for now, but I don't see any particular reason we can't do all the UnmarkGraying here, too.  It will cause additional work up front in the CC, but if we know all the objects it ungrays are going to be added to the CC graph anyways, it is probably going to reduce the time it takes overall.  Something to think about for a followup patch.

@@ +1548,5 @@
> +void
> +nsGlobalWindow::UnmarkGrayTimers()
> +{
> +  for (nsTimeout* timeout = FirstTimeout();
> +       timeout && IsTimeout(timeout);

So the timeout list stops being timeouts once there's any non-timeouts?

::: dom/base/nsJSEnvironment.h
@@ +197,5 @@
>  
> +  nsIScriptGlobalObject* GetCachedGlobalObject()
> +  {
> +    // Verify that we have a global so that this
> +    // does never return non-null value when GetGlobalObject() would.

The second line of the comment is a little unclear.  Maybe you mean "always returns a null when GetGlobalObject() is null"?

::: dom/src/events/nsJSEventListener.cpp
@@ +158,5 @@
> +      (!mScopeObject || !xpc_IsGrayGCThing(mScopeObject)) &&
> +      (!mHandler || !xpc_IsGrayGCThing(mHandler))) {
> +    nsIScriptGlobalObject* sgo =
> +      static_cast<nsJSContext*>(mContext.get())->GetCachedGlobalObject();
> +    return sgo && sgo->IsBlackForCC();

Why is the event listener not black when sgo is null?  Does that indicate the event listener is in some kind of shutting down state?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

> > +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(nsGlobalWindow)
> > +  return tmp->IsBlackForCC();
> 
> This is totally fine for now, but I don't see any particular reason we can't
> do all the UnmarkGraying here, too. 
CAN_SKIP_IN_CC runs in CC thread. unmarking js is not possible


> > +nsGlobalWindow::UnmarkGrayTimers()
> > +{
> > +  for (nsTimeout* timeout = FirstTimeout();
> > +       timeout && IsTimeout(timeout);
> 
> So the timeout list stops being timeouts once there's any non-timeouts?
Timeouts are stored in a circular linked list and IsTimeout check is need to
find the end of the list.


> > +  nsIScriptGlobalObject* GetCachedGlobalObject()
> > +  {
> > +    // Verify that we have a global so that this
> > +    // does never return non-null value when GetGlobalObject() would.
> 
> The second line of the comment is a little unclear.  Maybe you mean "always
> returns a null when GetGlobalObject() is null"?
That is what I mean, yes.


> 
> ::: dom/src/events/nsJSEventListener.cpp
> @@ +158,5 @@
> > +      (!mScopeObject || !xpc_IsGrayGCThing(mScopeObject)) &&
> > +      (!mHandler || !xpc_IsGrayGCThing(mHandler))) {
> > +    nsIScriptGlobalObject* sgo =
> > +      static_cast<nsJSContext*>(mContext.get())->GetCachedGlobalObject();
> > +    return sgo && sgo->IsBlackForCC();
> 
> Why is the event listener not black when sgo is null?
Because there might be non-cached-gray global

Comment 3

[Andrew McCreight [:mccr8]]

Thanks for the explanations!

(In reply to Olli Pettay [:smaug] from comment #2)
> > > +NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(nsGlobalWindow)
> > > +  return tmp->IsBlackForCC();
> > 
> > This is totally fine for now, but I don't see any particular reason we can't
> > do all the UnmarkGraying here, too. 
> CAN_SKIP_IN_CC runs in CC thread. unmarking js is not possible

Okay, I didn't realize that, but it isn't too surprising.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/7aef3dcb28d0