Add nsJSEventListener and nsGlobalWindow to BBP

RESOLVED FIXED in mozilla12

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

12 Branch
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Created attachment 591230 [details] [diff] [review]
patch
Attachment #591230 - Flags: review?(continuation)
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?
Attachment #591230 - Flags: review?(continuation) → review+
(Assignee)

Comment 2

5 years ago
> > +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
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.
(Assignee)

Updated

5 years ago
Blocks: 705582
(Assignee)

Comment 4

5 years ago
Created attachment 591789 [details] [diff] [review]
patch
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/mozilla-central/rev/7aef3dcb28d0
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
OS: Linux → All
Hardware: x86_64 → All
Summary: Add nsJSEventListener and nsGlobalWndow to BBP → Add nsJSEventListener and nsGlobalWindow to BBP
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.