346184 - If urlclassifier.sqlite is removed, db is not repopulated

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

After fixing bug 334174 and purposely corrupting the urlclassifier.sqlite db, the database is deleted and a new one is opened.  However, the new database stays at 0-size, never getting populated.  My guess is that safe browsing records that it downloaded the data once and then doesn't do so again, maybe?

Comment 1

[Tony Chang (Google)]

(some notes to myself)

Anti-phishing keeps track of table version numbers in user prefs.  So if the DB is corrupt and we remove it, we should delete those prefs.

Look around NS_ERROR_FILE_CORRUPTED in nsUrlClassifierDBService.cpp.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

I'd suggest using TableExists() to check for the presence of one of the url classifier's data tables, and nuking the prefs if they're not present.  That way you cover the case of urclassifier.sqlite being removed by the user as well

Comment 3

[Tony Chang (Google)]

Attachment: v1: check db tables before sending an update request

This has to be done async because the db is on one thread but we can only clear prefs from the main thread.

I also removed the callback wrapper since we don't need it anymore.  Also removed checkMac_ from listmanager.js since we don't use it either.

Comment 4

[Robert Strong (they/them - no direct email)]

Tony, some virus scanners appear to be picking up urlclassifier.sqlite-journal due to the phishing urls
http://forums.mozillazine.org/viewtopic.php?t=445171

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

But they don't pick up urlclassifier.sqlite? curious, I guess they're just watching for file creation or something?

This could be pretty painful for the release, though. =/

Comment 6

[Tony Chang (Google)]

(In reply to comment #4)
> Tony, some virus scanners appear to be picking up urlclassifier.sqlite-journal
> due to the phishing urls
> http://forums.mozillazine.org/viewtopic.php?t=445171

This is bug 329715.  I tried reproducing using AntiVir, but can't seem to get it to trigger.  I'll try again on windows.

Comment 7

[Darin Fisher]

Comment on attachment 231168 [details] [diff] [review]
v1: check db tables before sending an update request

>Index: toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp

>+    const nsDependentCSubstring &tableName = Substring(tableNames, cur,
>+                                                       next - cur);

For things like this, if you want to type slightly less, I think you
can just do:

      const nsCSubstring &tableName = Substring(...)

That also nicely hides the "nsDependentCSubstring" type, which I believe
is  somewhat of an implementation detail.

r=darin

Comment 8

[Tony Chang (Google)]

Attachment: v2: nsDependentCSubstring -> nsCSubstring

Comment 9

[Tony Chang (Google)]

on trunk

Comment 10

[Tony Chang (Google)]

Attachment: bonecho patch

Patch for bonecho.  Still keeps the callback wrapper since bug 337492 is targetted at moz 1.9.

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 231641 [details] [diff] [review]
bonecho patch

a=mconnor on behalf of drivers for checkin to 1.8 branch for 1.8.1

Comment 12

[Tony Chang (Google)]

on branch