1345464 - Add an optional EventTarget to nsExpirationTracker

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

This is to support DocGroup labeling for the TimerCallback called internally in nsExpirationTracker:
http://searchfox.org/mozilla-central/source/xpcom/ds/nsExpirationTracker.h#363

The high frequent one using this is the LayerActivityTracker in layout/:
http://searchfox.org/mozilla-central/source/layout/painting/ActiveLayerTracker.cpp#124
according to the analysis in bug 1333984 comment 10.

We should fix this one first to allow each module owner to support DocGroup labeling for these use cases:
http://searchfox.org/mozilla-central/search?q=symbol:T_nsExpirationTracker&redirect=false

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

Just realized that all the use cases of nsExpirationTracker are singleton.

Bill, 
was this the reason why we didn't have a bug for this after the analysis of bug 1333984 and will keep this unlabeled?

Note: I found this while looking for "LayerActivityTracker" whose frequency is high in the analysis.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

Per-offline discussion, *SystemGroup* could be an option to these ExpirationTracker singletons if they just do some internal resource management without touching anything to the web content such as js callback.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Looking at LayerActivityTracker, I'm not sure if it's safe to label it as SystemGroup.

Daniel, suppose that we were in the middle of a reflow (maybe at the top of nsBlockFrame::Reflow) and we interrupted it and started running LayerActivityTracker::NotifyExpired. Would that cause problems? I'm mostly worried about the SchedulePaint call, which seems to do some SVG stuff.

If we can't use SystemGroup, I guess we'll have to use different LayerActivityTrackers for each tab or something.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Note: The task for labeling LayerActivityTracker will be tracked in bug 1345387.
I'll provide a patch first in this bug to allow EventTarget being set optionally per ExpirationTracker.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Bill McCloskey (:billm) from comment #3)
> Looking at LayerActivityTracker, I'm not sure if it's safe to label it as
> SystemGroup.
> 
> Daniel, [...]

Looks like I should reply over on bug 1345387, per comment 4.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch: Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM.

This patch is to allow nsExpirationTracker::TimerCallback to be invoked on specified EventTarget to support Quantom-DOM labeling.

Patch to be uploaded in bug 1346145 shows the example for setting a DocGroup EventTarget to the SelectorCache per nsDocument instance.

Hi :Ms2ger,

May I have your review for this change?

Thanks!

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8846513 [details] [diff] [review]
Patch: Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM.

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

I don't feel competent to review this change, sorry.

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8846513 [details] [diff] [review]
Patch: Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM.

(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #7)
> I don't feel competent to review this change, sorry.
Never mind!
It seems that Nathan is back who has done several review on this file before.

Hi Nathan,

May I have your review for this change?

Thanks!

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8846513 [details] [diff] [review]
Patch: Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM.

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

r- for the implementation, not the idea.

::: xpcom/ds/nsExpirationTracker.h
@@ +88,2 @@
>     */
> +  explicit nsExpirationTracker(uint32_t aTimerPeriod, const char* aName,

Nit: you no longer need `explicit` on this constructor, as it takes multiple arguments.

@@ +367,5 @@
>      if (!mTimer) {
>        return NS_ERROR_OUT_OF_MEMORY;
>      }
> +    if (mEventTarget) {
> +      mTimer->SetTarget(mEventTarget);

This change introduces UAF races: the execution of the timer's callback may race with the destruction of the nsExpirationTracker it's trying to use.

...but this may be safe assuming that the timer is only ever created on the main thread, and the passed-in event target is only ever on the main thread.  So we would need some kind of assertions here; I think they should be release assertions.  It would be great if those assertions could be moved out of templated header code into a separate .cpp, but I think that work could wait for a followup bug.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Nathan Froyd [:froydnj] from comment #9)
> This change introduces UAF races: the execution of the timer's callback may
> race with the destruction of the nsExpirationTracker it's trying to use.
Thanks for capturing this. I'll address this problem in next update.

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v2) Patch: Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM.

Given the facts that:
1. nsExpirationTracker can only be constructed on the main thread.
   (The internal ExpirationTrackerObserver can only be Init/Destroyed on the
    main thread.)
2. There are some potential races in SurfaceCache that creates tracker 
   on the main thread but calling AddObject/RemoveObject off the main thread
   without a mutex lock [1]. :-(
hance, the assertions is added in construction time instead of the time when
CheckStartTimer() is called to ensure that the passed-in event target is always
on the main thread.

May I have your review again for this change?

Thanks!

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=83931726&repo=try&lineNumber=2508

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8847514 [details] [diff] [review]
(v2) Patch: Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM.

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

I'm having a hard time convincing myself that requiring a main-thread event target is going to solve things; as your try run shows, we're calling AddObject/RemoveObject on different threads anyway.  If you provide a main-thread event target for such cases, you may be introducing races (or making them worse), because the timer will now be running in a completely unexpected place.  Asserting same-thread-ness in AddObject/RemoveObject would probably be too expensive, though. =/  I'm not sure what a good solution is here.

::: xpcom/ds/nsExpirationTracker.h
@@ +102,5 @@
> +      bool current = false;
> +      MOZ_RELEASE_ASSERT(
> +        NS_SUCCEEDED(mEventTarget->IsOnCurrentThread(&current)) && current,
> +        "We are in a race of calling TimerCallback with the destruction of the"
> +        " nsExpirationTracker it's trying to use");

This assertion message doesn't really work for this location.  How about simply: "provided event target must be on the main thread"?

Comment 14

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Nathan Froyd [:froydnj] from comment #13)
> Comment on attachment 8847514 [details] [diff] [review]
> (v2) Patch: Add an optional EventTarget to nsExpirationTracker to support
> Labeling for Quantum-DOM.
> 
> Review of attachment 8847514 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm having a hard time convincing myself that requiring a main-thread event
> target is going to solve things; as your try run shows, we're calling
> AddObject/RemoveObject on different threads anyway.  If you provide a
> main-thread event target for such cases, you may be introducing races (or
> making them worse), because the timer will now be running in a completely
> unexpected place.  Asserting same-thread-ness in AddObject/RemoveObject
> would probably be too expensive, though. =/  I'm not sure what a good
> solution is here.
> 
Asserting same-thread-ness with NS_DECL_OWNINGTHREAD/NS_ASSERT_OWNINGTHREAD was 
tested locally when preparing this patch v2 and the passed-in EvenTarget shall 
be on the same thread as well.
However, the improper use cases in SurfaceCache block me to enable it.
IMO, calling AddObject/RemoveObject on different threads is the wrong use case,
because it implicitly creates a TimerCallback to be run on different thread
which introduces the race you mentioned earlier that the TimerCallback could be
run after the destruction of its tracker.
That's some thing must be fixed in SurfaceCache but it's out-of-socpe of this bug.

Hence, for now, what I can do is to restrict the EventTarget to be on the main 
thread instead of the owning thread of a tracker since it can only be created on
main thread.

Someday, if someone wants to support labeling on SurfaceCache, this assertion
will tell them to fix the improper use of tracker off-main thread.

Does this make sense to you?

> ::: xpcom/ds/nsExpirationTracker.h
> @@ +102,5 @@
> > +      bool current = false;
> > +      MOZ_RELEASE_ASSERT(
> > +        NS_SUCCEEDED(mEventTarget->IsOnCurrentThread(&current)) && current,
> > +        "We are in a race of calling TimerCallback with the destruction of the"
> > +        " nsExpirationTracker it's trying to use");
> 
> This assertion message doesn't really work for this location.  How about
> simply: "provided event target must be on the main thread"?
Sure, but I'd like to add some comment to talking about the race we want to prevent
in the code. Maybe I could add additional comment: "SurfaceCache is a *known* use
case that doesn't ensure the same-thread-ness properly when access this tracker."
Is that okay to you?

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #14)
> Hence, for now, what I can do is to restrict the EventTarget to be on the
> main 
> thread instead of the owning thread of a tracker since it can only be
> created on
> main thread.
Sorry for missing this explanation:
To prevent thing get worst in SurfaceCache,
Same assertion of the following has to be added in CheckStartTimer() if mEventTarget is set:
if (mEventTarget) {
  bool current = false;
  MOZ_RELEASE_ASSERT(
    NS_SUCCEEDED(mEventTarget->IsOnCurrentThread(&current)) && current,
      "We are in a race of calling TimerCallback with the destruction of the"
      " nsExpirationTracker it's trying to use");
  mTimer->SetTarget(mEventTarget);
}
> Someday, if someone wants to support labeling on SurfaceCache, this assertion
> will tell them to fix the improper use of tracker off-main thread.

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v3) Patch: Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM

1. Add NS_WARNING_ASSERTION to check if tacker's method is called off the main thread.
2. Add MOZ_RELEASE_ASSERT in CheckStartTimer if mEventTarget is set to prevent subclass like SurfaceCache getting worse when runnable labeling is needed on it.

Will this make more sense to you?

Comment 17

[Nathan Froyd [:froydnj]]

Comment on attachment 8848058 [details] [diff] [review]
(v3) Patch: Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM

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

::: xpcom/ds/nsExpirationTracker.h
@@ +350,5 @@
> +#ifdef MOZ_THREAD_SAFETY_OWNERSHIP_CHECKS_SUPPORTED
> +    // Potential race conditions:
> +    // 1. Run TimerCallback while tracker is destroyed on the owning thread.
> +    // 2. Thread-safe problem in accessing the cached objects.
> +    NS_WARNING_ASSERTION(

Did you test this?  If SurfaceCache is triggering this, and if tests use it widely, it will a) generate a lot of logspam, and b) will cause some tests that care about the number of assertions to fail--possibly intermittently.  I would like to have this hear, but I'm unsure that it's the right way to address it.

@@ +411,5 @@
> +      bool current = false;
> +      MOZ_RELEASE_ASSERT(
> +        NS_SUCCEEDED(mEventTarget->IsOnCurrentThread(&current)) && current,
> +        "You are in a race of calling TimerCallback with the destruction of the"
> +        " tracker on different threads");

How about:

"Starting timer on non-main thread causes races between timer execution and destruction of the tracker"

?

Comment 18

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Nathan Froyd [:froydnj] from comment #17)
> Did you test this?  If SurfaceCache is triggering this, and if tests use it
> widely, it will a) generate a lot of logspam, and b) will cause some tests
> that care about the number of assertions to fail--possibly intermittently. 
> I would like to have this hear, but I'm unsure that it's the right way to
> address it.
By taking one mochitest job on linux as example, there is about 200 logs on SurfaceTracker:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7ddee2237a2ff4d4a71e61ad1ef7e13e041b2cc&selectedJob=84296835 
But I didn't see test failure due to this logspam.

However, I could just add the warning in CheckStartTimer() for now to reduce the spam.

> @@ +411,5 @@
> > +      bool current = false;
> > +      MOZ_RELEASE_ASSERT(
> > +        NS_SUCCEEDED(mEventTarget->IsOnCurrentThread(&current)) && current,
> > +        "You are in a race of calling TimerCallback with the destruction of the"
> > +        " tracker on different threads");
> 
> How about:
> 
> "Starting timer on non-main thread causes races between timer execution and
> destruction of the tracker"
> 
> ?

After thinking twice, I start to wonder if the MOZ_RELEASE_ASSERT in CheckStartTimer() is really meaningful.
1. We have already ensured that the provided event target is on the main thread in the constructor.
2. SurfaceCache shouldn't get worse if a main-thread event-target is provided because that makes the TimerCallback be invoked persistently on main thread which sounds better than what happens now on SurfaceCache.

Maybe we should just add additional logic in CheckStartTimer() as followed instead of these assertions to ensure that the TimerCallback will always be triggered on the main thread even CheckStartTimer() was called off the main thread:
> nsresult CheckStartTimer()
> {
>   // XXX: Potential race conditions:
>   // 1. Run TimerCallback while the tracker is destroyed on the owning thread.
>   //    We work around this by setting the main-thread as the event target of
>   //    the timer if this method is called off the main thread.
>   // 2. Thread-safe problem of accessing the cached objects.
>   NS_WARNING_ASSERTION(NS_IsMainThread(),
>     nsPrintfCString("%s is not thread-safe: Starting timer on non-main thread"
>       " causes races between timer execution and destruction of the tracker",
>       mName).get());
> 
>   if (mTimer || !mTimerPeriod) {
>     return NS_OK;
>   }
>   mTimer = do_CreateInstance("@mozilla.org/timer;1");
>   if (!mTimer) {
>     return NS_ERROR_OUT_OF_MEMORY;
>   }
>   // TimerCallback should always be run on the main thread to prevent races
>   // to the destruction of the tracker.
>   nsCOMPtr<nsIEventTarget> target =
>     mEventTarget ? mEventTarget : do_GetMainThread();
>   NS_ENSURE_STATE(target);
>   mTimer->SetTarget(target);
>   mTimer->InitWithNamedFuncCallback(TimerCallback, this, mTimerPeriod,
>                                     nsITimer::TYPE_REPEATING_SLACK, mName);
>   return NS_OK;
> }

I'll do some test for this change before next review. :)

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v4) Patch: Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM

Revise patch according to the analysis in comment 18:
1. Add NS_WARNING_ASSERTION only in CheckStartTimer() to reduce log spams.
2. Ensure that TimerCallback() will be called from an EventTarget on the main thread even if CheckStartTimer() is called off the main thread.

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

update treeherder result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef18a82d42f7e0f9bb471b9b29ba57e3d5fa10c9

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 8849512 [details] [diff] [review]
(v4) Patch: Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM

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

I'm starting to think that either you or somebody else will just have to make nsExpirationTracker thread-safe in some fashion.

::: xpcom/ds/nsExpirationTracker.h
@@ +386,5 @@
>      if (!mTimer) {
>        return NS_ERROR_OUT_OF_MEMORY;
>      }
> +    // TimerCallback should always be run on the main thread to prevent races
> +    // to the destruction of the tracker.

Well, yes, but this opens other possibilities: before, if the timer was started on a non-main-thread and all the modifications to the tracker were made on that thread, the tracker was still racy, but at least the timer and the modifications made weren't going to race against each other.

With this change, you're trading one race for another: modifications made in the timer callback will race against modifications made in Add/Remove/etc.  That doesn't seem like a good tradeoff.

@@ +390,5 @@
> +    // to the destruction of the tracker.
> +    nsCOMPtr<nsIEventTarget> target =
> +      mEventTarget ? mEventTarget : do_GetMainThread();
> +    NS_ENSURE_STATE(target);
> +    mTimer->SetTarget(target);

Note that this is inefficient when we're already on the main thread, because the timer will already be set up that way.

Comment 22

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: (v5) Patch: Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM.

Update patch according to the change in bug 1350177.

Treeherder result for reference:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=062194d9d3bbffacd59b0cb5bc07f30b497af2ff

Comment 23

[Nathan Froyd [:froydnj]]

Comment on attachment 8853132 [details] [diff] [review]
(v5) Patch: Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM.

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

Thanks for working through all of this!

Comment 24

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Nathan Froyd [:froydnj] from comment #23)
> Thanks for working through all of this!
That's my pleasure.
I really learnt a lot from these change especially the template stuff. :)

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/899fd5a47a20
Add an optional EventTarget to nsExpirationTracker to support Labeling for Quantum-DOM. r=froydnj

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/899fd5a47a20