1346757 - DownloadCallback should be fired after all valid updates are applied

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, enhancement)

Description

[Henry Chang [:hchang]]

If an update contains a invalid forward URL, the timing that downloadCallback is
fired is in [1] or [2], when the "direct" update in the top level update
will still be applied later [3].

In the real world use case it doesn't matter whether we are notified 
"download error" before or after the other updates are applied, especially
the further requests will be blocked by the updates on worker thread.
However, we are going to offload the update to update thread. The internal
state when we are notified "download error" is important. Take test case [4]
for example. [4] contains a valid update in the top level and a invalid
forward URL. The current expected behavior is a little weird: we expect
the "tableData" in the state "before" the update but the "lookup result"
is after the update (remember there is still an valid update in the top
level.) I am a little surprised that there is no intermittent error 
around here. (maybe the DBService::FinishUpate() will deterministically
be called between DBService::GetTables() and DBService::Lookup())

So, my idea is to do notify "download error" in updateSuccess or
updateError. This can ensure any valid update has already been
applied when we get notified "download error". (actually we intentionally
skip callback in updateSuccess/updateError, which implies they
will be notified no matter what.)

[1] http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#711
[2] http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#360
[3] http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#783
[4] http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/toolkit/components/url-classifier/tests/unit/test_streamupdater.js#95
[5] http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#711
[6] http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp#785
[7] http://searchfox.org/mozilla-central/rev/ef0b6a528e0e7b354ddf8cdce08392be8b8ca679/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#642

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug 1346757 - Change the downloadError callback timing.

Review commit: https://reviewboard.mozilla.org/r/119644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/119644/

Comment 2

[François Marier [:francois]]

Comment on attachment 8846624 [details]
Bug 1346757 - Change the downloadError callback timing.

https://reviewboard.mozilla.org/r/119644/#review121700

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a26b45336e56
Change the downloadError callback timing. r=francois

Comment 4

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42c076ebf07f

Comment 5

[Carsten Book [:Tomcat]]

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

Comment 6

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ee7517319ef7

Comment 7

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaf93e924ff4