1338691 - Add a pref for delaying tracking timeout throttling after the page load finishes

Status: RESOLVED FIXED

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

Description

[(no longer active)]

During manual testing of the prefs added in bug 1325467, we found out that sometimes throttling the tracking timeouts can cause ads to load with a delay.  This is visible on cnn.com for example.

After a long time debugging this, I didn't really find any better solution than just delaying the throttling to a few seconds after the page load is finished.  I have added a new pref for this purpose: dom.timeout.tracking_throttling_delay

Setting this pref to a positive integer N causes tracking timeouts to only start being throttled after N milliseconds after the page load has finished (using the load event as the indicator).

Comment 1

[(no longer active)]

Attachment: Add a pref for delaying tracking timeout throttling after the page load finishes

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8836236 [details] [diff] [review]
Add a pref for delaying tracking timeout throttling after the page load finishes

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

Looks reasonable, but I think we should use nsITimerCallback instead of abusing nsIRunnable here.  I believe this does require defining a class, but if you really wanted to you could make something like NewRunnableMethod() or that takes closures for nsITimerCallback.

::: dom/base/TimeoutManager.cpp
@@ +138,5 @@
>  
>  TimeoutManager::~TimeoutManager()
>  {
> +  if (mThrottleTrackingTimeoutsTimer) {
> +    mThrottleTrackingTimeoutsTimer->Cancel();

I think we should be able to assert in this case that:

MOZ_DIAGNOSTIC_ASSERT(!mThrottleTrackingTimeouts)

@@ +1226,5 @@
> +TimeoutManager::StartThrottlingTrackingTimeouts(nsGlobalWindow* aWindow)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(aWindow == &mWindow);
> +  MOZ_ASSERT(mThrottleTrackingTimeoutsTimer);

Can you make low cost assertions (not the IsMainThread one) MOZ_DIAGNOSTIC_ASSERT()?  It really helps catch more bugs due to increased coverage with opt builds.

@@ +1231,5 @@
> +
> +  MOZ_LOG(gLog, LogLevel::Debug,
> +          ("TimeoutManager %p started to throttle tracking timeouts\n", this));
> +
> +  mThrottleTrackingTimeouts = true;

Before this I think we should be able to assert:

MOZ_DIAGNOSTIC_ASSERT(!mThrottleTrackingTimeouts)

@@ +1238,5 @@
> +
> +static void
> +ThrottleTrackingTimeoutsCallback(nsITimer* aTimer, void* aClosure)
> +{
> +  nsCOMPtr<nsIRunnable> r = dont_AddRef(static_cast<nsIRunnable*>(aClosure));

This feels a little weird to me.  Rather than playing games with the ref count on a runnable wouldn't it be more explicit to create an nsITimerCallback object?  Then the nsITimer could explicitly AddRef() it.

@@ +1245,5 @@
> +
> +void
> +TimeoutManager::OnDocumentLoaded()
> +{
> +  if (gTrackingTimeoutThrottlingDelay > 0) {

nit: Please use short-circuit logic:

if (gTrackingTimeoutThrottlingDelay < 0) {
  return;
}

@@ +1246,5 @@
> +void
> +TimeoutManager::OnDocumentLoaded()
> +{
> +  if (gTrackingTimeoutThrottlingDelay > 0) {
> +    MOZ_ASSERT(!mThrottleTrackingTimeouts);

MOZ_DIAGNOSTIC_ASSERT()

@@ +1252,5 @@
> +    MOZ_LOG(gLog, LogLevel::Debug,
> +            ("TimeoutManager %p delaying tracking timeout throttling by %dms\n",
> +             this, gTrackingTimeoutThrottlingDelay));
> +
> +    mThrottleTrackingTimeoutsTimer =

Before this assert:

MOZ_DIAGNOSTIC_ASSERT(!mThrottleTrackingTimeoutsTimer);

I think its impossible for two load events to fire for the same document.  Even `document.open()` should not do it because a new inner window is created.

@@ +1262,5 @@
> +    // We use a runnable in order to use the helper methods to keep the window
> +    // alive.
> +    nsCOMPtr<nsIRunnable> r =
> +      NewNonOwningRunnableMethod<StoreRefPtrPassByPtr<nsGlobalWindow>>(this,
> +        &TimeoutManager::StartThrottlingTrackingTimeouts, &mWindow);

Can you tweak the comment text here to say something more like "We must hold a strong ref to the window in order to keep the timer object alive."?

Comment 3

[(no longer active)]

Attachment: Add a pref for delaying tracking timeout throttling after the page load finishes

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8836854 [details] [diff] [review]
Add a pref for delaying tracking timeout throttling after the page load finishes

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

::: dom/base/TimeoutManager.cpp
@@ +1230,5 @@
>  }
> +
> +namespace {
> +
> +class ThrottleTrackingTimeoutsCallback final : public nsITimerCallback

Thanks!

::: dom/base/TimeoutManager.h
@@ +213,5 @@
>  
>    int32_t                     mBackPressureDelayMS;
>  
> +  nsCOMPtr<nsITimer>          mThrottleTrackingTimeoutsTimer;
> +  bool                        mThrottleTrackingTimeouts;

Please put the bool at the end to avoid padding in the object layout.

Comment 5

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #4)
> ::: dom/base/TimeoutManager.h
> @@ +213,5 @@
> >  
> >    int32_t                     mBackPressureDelayMS;
> >  
> > +  nsCOMPtr<nsITimer>          mThrottleTrackingTimeoutsTimer;
> > +  bool                        mThrottleTrackingTimeouts;
> 
> Please put the bool at the end to avoid padding in the object layout.

It already is at the end, the static after it doesn't participate in object layout...

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari from comment #5)
> It already is at the end, the static after it doesn't participate in object
> layout...

If you want to be all technical, sure. :-)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1beb916956b
Add a pref for delaying tracking timeout throttling after the page load finishes; r=bkelly

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #2)
> > +  nsCOMPtr<nsIRunnable> r = dont_AddRef(static_cast<nsIRunnable*>(aClosure));
> 
> This feels a little weird to me.  Rather than playing games with the ref
> count on a runnable wouldn't it be more explicit to create an
> nsITimerCallback object?  Then the nsITimer could explicitly AddRef() it.

BTW, thanks for doing this.  I realized another reason to avoid these explicit AddRef()/Release() calls is that its really easy to leak if the timer is canceled.  Since the callback is not fired the Release() is not called there.  You can try to manually Release() every time you Cancel(), but you have to be careful not to double Release() if the callback does in fact fire even though you called Cancel().

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c1beb916956b

Comment 10

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #8)
> (In reply to Ben Kelly [:bkelly] from comment #2)
> > > +  nsCOMPtr<nsIRunnable> r = dont_AddRef(static_cast<nsIRunnable*>(aClosure));
> > 
> > This feels a little weird to me.  Rather than playing games with the ref
> > count on a runnable wouldn't it be more explicit to create an
> > nsITimerCallback object?  Then the nsITimer could explicitly AddRef() it.
> 
> BTW, thanks for doing this.  I realized another reason to avoid these
> explicit AddRef()/Release() calls is that its really easy to leak if the
> timer is canceled.  Since the callback is not fired the Release() is not
> called there.  You can try to manually Release() every time you Cancel(),
> but you have to be careful not to double Release() if the callback does in
> fact fire even though you called Cancel().

Yeah good point, I was actually surprised why this didn't cause leaks on try, and I think the reason was that given the window was being held alive by the timer, we'd never actually fall into a case where the timer was canceled (so the code calling Cancel() was essentially dead code.)  This made me realize that the patch that I landed actually has the exact same issue, since we're still holding the window alive by the timer, so for example if you set the value of this pref to 10,000, all windows are kept alive for a minimum of 10 seconds, which seems terrible.  :(

I'll fix this in a follow-up.  Sorry for not being super careful...

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

But don't we cancel the timers when the window is suspended?

https://dxr.mozilla.org/mozilla-central/source/dom/base/TimeoutManager.cpp#1084

This definitely happens when the window is put in the bfcache.  Not sure about when a window bypasses the bfcache, though.

Comment 12

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #11)
> But don't we cancel the timers when the window is suspended?
> 
> https://dxr.mozilla.org/mozilla-central/source/dom/base/TimeoutManager.
> cpp#1084

No this is a different timer.  Right now I don't even call Cancel() on it anywhere. <https://hg.mozilla.org/mozilla-central/rev/c1beb916956b#l1.205>