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

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.

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.

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.

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.

(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.

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 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 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 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.

(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.

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 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"?

(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?

(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.

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 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" ?

(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. :)

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.

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

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.

Update patch according to the change in bug 1350177. Treeherder result for reference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=062194d9d3bbffacd59b0cb5bc07f30b497af2ff

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!

(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. :)

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