1110891 - listmanager.js and nsUrlClassifierStreamUpdater both set timers to fetch updates

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

Both of these modules set timers to fetch updates on success. This seems like a bug.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/content/listmanager.js#405


404   this.updateCheckers_[updateUrl] =
405     new G_Alarm(BindToObject(this.checkForUpdates, this, updateUrl),
406                 delay, false);

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#352

350   // Wait the requested amount of time before starting a new stream.
351   nsresult rv;
352   mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
353   if (NS_SUCCEEDED(rv)) {
354     rv = mTimer->InitWithCallback(this, requestedDelay,
355                                   nsITimer::TYPE_ONE_SHOT);
356   }

It looks like variants of both of these have been present in both modules since 2007 or 2008.

Comment 1

[leichixian]

Is this the reason for crash report https://crash-stats.mozilla.com/report/index/edffde9f-a8cd-4294-a909-d9d732141226 and https://crash-stats.mozilla.com/report/index/ed0b0df5-e7d3-4eff-9fae-f00032141226 ? All of these two crashes happen when I close nightly and it suddenly crashed.

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

It may be -- we should try deleting the timer code in the streamupdater and seeing if that works.

Comment 3

[François Marier [:francois]]

Permalinks to replace Monica's (now broken) mxr links:

https://dxr.mozilla.org/mozilla-central/rev/6adc822f5e27a55551faeb6c47a9bd8b0859a23b/toolkit/components/url-classifier/content/listmanager.js#454-456
https://dxr.mozilla.org/mozilla-central/rev/6adc822f5e27a55551faeb6c47a9bd8b0859a23b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#355-362

Comment 4

[François Marier [:francois]]

We should probably clean this up before we refactor the code and add V4 support.

Comment 5

[Henry Chang [:hchang]]

From what I investigated:

The timer set in |nsUrlClassifierStreamUpdater::StreamFinished| is to fetch the "forward" updates [1] (i.e. redirection) since the timer callback "FetchNext" only makes request for "mPendingUpdates", which would be appended only in |nsUrlClassifierStreamUpdater::UpdateUrlRequested| [2] and it is called only when a forward URL is parsed in ProtocolParser [3]. 

To sum up, the timer considered duplicated in |nsUrlClassifierStreamUpdater::StreamFinished| is to deal with different URLs from what listmanager does.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#531
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#289
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/ProtocolParser.cpp#137

Comment 6

[François Marier [:francois]]

That's great Henry, thanks for digging into this!

I guess all we need to do to close this bug is to replace this comment:

  // This appears to be a duplicate timer (see bug 1110891)

in https://dxr.mozilla.org/mozilla-central/rev/6adc822f5e27a55551faeb6c47a9bd8b0859a23b/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#356.

Maybe with something like this:

  // This timer is for fetching indirect updates ("forwards") from any "u:" lines
  // that we encountered while processing the server response. It is NOT for
  // scheduling the next time we pull the list from the server. That's a different
  // timer in listmanager.js (see bug 1110891).

That way we don't accidentally remove one of the two or get confused again.

Comment 7

[Henry Chang [:hchang]]

Attachment: Patch to change comment

Comment 8

[Gian-Carlo Pascutto [:gcp]]

(In reply to Henry Chang [:henry] from comment #5)
> From what I investigated:
> 
> The timer set in |nsUrlClassifierStreamUpdater::StreamFinished| is to fetch
> the "forward" updates 


It does not make any sense that we would delay fetching the updates. The spec certainly has no such delay. And indeed: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#542

Maybe a Timer is simply not the right approach here?

Comment 9

[Henry Chang [:hchang]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #8)
> (In reply to Henry Chang [:henry] from comment #5)
> > From what I investigated:
> > 
> > The timer set in |nsUrlClassifierStreamUpdater::StreamFinished| is to fetch
> > the "forward" updates 
> 
> 
> It does not make any sense that we would delay fetching the updates. The
> spec certainly has no such delay. And indeed:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-
> classifier/nsUrlClassifierDBService.cpp#542
> 
> Maybe a Timer is simply not the right approach here?

If the callback |StreamFinished| is only designed for indirect fetch, we can just ignore the delay and do the fetch right away. What I am not sure is if this callback is also designed for other purpose (even though I don't see any in the codebase ...)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/0136b1e24160
Replace comment to clarify the different purposes of the two timers. r=francois

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0136b1e24160