779008 - url-classifier should handle addition of test-malware-simple / test-phish-simple entries

Status: RESOLVED DUPLICATE of bug 1531354

(Toolkit :: Safe Browsing, defect, P2)

Description

[Justin Dolske [:Dolske]]

Blah. Bug 778855 is a performance regression from the landing of SafeBrowsing.jsm.

I suspect one way to fix it -- which we'd want to do anyway, hence this bug -- is to eliminate SafeBrowsing.jsm's addMozEntries(), and instead have the url-classifier code deal with it (eg, add once during first DB creation). As-is, and even pre-refactor, I suspect that the front-end code adding these test entries is resulting in disk IO (writes) every single startup. The refactoring just made it happen earlier (withing our startup measurement timespan), hence the regression.

But doing this is a little tricky, and I'm not sure what our long term plans are...

We could add these entries when the DB is first created. (Existing DBs can be assumed to already have these entries, since we always add them.) I haven't looked at the prefix code stuff to see if/when/where we'd need to handle these. I assume nothing is ever clearing these special entries.

The other thought is to simplify things, and special-case these two URLs in ::Lookup(). That would mean visiting a test URL isn't actually exercising any significant portion of the url-classifier code. But I'm not sure we really care... The purpose of these pages is just for demoing the UI without risk of getting pwned if you exercise the "ignore warning". And they're already special cases (eg, having them blocked doesn't mean safebrowsing updates are working). I'd suspect there's a gob of extra code that does lookups on these two testing tables which could be entirely eliminated.

GCP: Have an opinion here?

Comment 1

[Justin Dolske [:Dolske]]

FTR, the two URLs are:

  http://www.mozilla.org/firefox/its-an-attack.html

and

  http://www.mozilla.org/firefox/its-a-trap.html

Comment 2

[Justin Dolske [:Dolske]]

Oh, another (poor?) option would be to use nsIUrlClassifierDBService :: GetTables() to see if the test entries (tables) already exist, and if so skip adding them again. That might help with startup perf, but it would still be causing DB reads when not needed.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

So, the problem of having the DB entries being added in the DB creating code is that if we want to move them around (which was happened in the past and is planned to happen in the future), we have to do a check (at startup? same problem again...) to update/migrate the database if the ones we have are still in the wrong location, or alternatively increment the DB version number so it gets blown away automagically (which isn't very nice either).

Special casing these in Lookup indeed has the problem that it's not exercising the code it's purporting to test, and it isn't going to help much to make things simpler or cleaner either. 

>I'd suspect there's a gob of extra code that does lookups on these two testing 
>tables which could be entirely eliminated.

Nope, they use the same codepath as our current tests for updates, and the code path for lookups is entirely generic. It wouldn't save anything.

>Oh, another (poor?) option would be to use nsIUrlClassifierDBService :: 
>GetTables() to see if the test entries (tables) already exist, and if so skip 
>adding them again but it would still be causing DB reads when not needed.

The new UrlClassifier code caches existing tables, so the disk read would not be lost. But you have the same problem if you ever need to update the URLs.

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Do we have some codepath for the first run of a new Firefox version and/or profile?

We could put the creation of those tables there, still after a sufficient delay. If we ever need to change the contents, we can rename the table test-malware -> test-malware1 (and optionally delete the old files).

Comment 5

[:Gavin Sharp [email: [e-mail]]]

nsBrowserGlue's _migrateUI could be used for this, but it's called rather early so you'd probably want to delay the actual work.

Comment 6

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 7

[Dimi Lee [ooo 16.04 - 21.04］]

Hi gcp,
I am thinking if we can change the table name of those test entries to something like test-memory-xxx-simple and we don't do any file read/write for those "memory" tables (Right now, I can't think of any reason those tables need to have a disk copy).
I think if this works, it is even better than generate the table at the first run because that still requires file I/O every startup which seems to cause a performance problem in certain devices(Bug 1353956 Comment 30)

How do you think?

Comment 8

[Gian-Carlo Pascutto [:gcp]]

The only reason for those tables to have a disk copy is that they are used in our tests and this means that the tests exercise the disk reading/writing code just like normal usage would.

But I think that in the time since this bug, the test coverage of SafeBrowsing code has been greatly extended via unit tests so that it's sufficient and not writing those tables is not big deal.

Comment 9

[Dimi Lee [ooo 16.04 - 21.04］]

This is now fixed by Bug 1531354, test entries are now added directly in LookupCache without consuming disk IO