Closed Bug 1346757 Opened 5 years ago Closed 5 years ago

DownloadCallback should be fired after all valid updates are applied

Categories

(Toolkit :: Safe Browsing, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: hchang, Assigned: hchang)

References

Details

Attachments

(1 file)

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
Blocks: 1339050
Assignee: nobody → hchang
Attachment #8846624 - Flags: review?(francois)
Comment on attachment 8846624 [details]
Bug 1346757 - Change the downloadError callback timing.

https://reviewboard.mozilla.org/r/119644/#review121700
Attachment #8846624 - Flags: review?(francois) → review+
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a26b45336e56
Change the downloadError callback timing. r=francois
https://hg.mozilla.org/mozilla-central/rev/a26b45336e56
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.