1305486 - Enable V4 update by default on Nightly only

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[François Marier [:francois]]

Once we have support for partial updates and RICE decoding,  we should enable V4 updates on Nightly only (not riding the train).

This will be in addition to doing V2 updates, not instead of. Google is fine with us doing these double updates in pre-release channels.

Lookups will happen in both the V2 and V4 code, but the V4 lookups are currently returning false since they're not implemented.

Comment 1

[Henry Chang [:hchang]]

Attachment: 0001-Bug-1305486-Add-googpub-phish-proto-goog-malware-pro.patch

Comment 2

[Henry Chang [:hchang]]

Attachment: Bug 1305486 - Enable v4 tables on nightly build. .

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

Comment 3

[François Marier [:francois]]

Comment on attachment 8812531 [details]
Bug 1305486 - Enable v4 tables on nightly build. .

https://reviewboard.mozilla.org/r/94240/#review94408

::: modules/libpref/init/all.js:5048
(Diff revision 1)
>  // Enable mapped array buffer by default.
>  pref("dom.mapped_arraybuffer.enabled", true);
>  
>  // The tables used for Safebrowsing phishing and malware checks.
> -pref("urlclassifier.malwareTable", "goog-malware-shavar,goog-unwanted-shavar,test-malware-simple,test-unwanted-simple");
> -
> +#ifdef NIGHTLY_BUILD
> +pref("urlclassifier.malwareTable", "goog-malware-shavar,goog-unwanted-shavar,test-malware-simple,test-unwanted-simple,goog-malware-proto,goog-unwanted-proto");

The `malwareTable` pref no longer gets set in non-nightly builds.

I would suggest doing `malwareTable` and `phishTable` in separate `#ifdef NIGHTLY_BUILD` to make it more readable.

Comment 4

[Henry Chang [:hchang]]

Comment on attachment 8812531 [details]
Bug 1305486 - Enable v4 tables on nightly build. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94240/diff/1-2/

Comment 5

[François Marier [:francois]]

Comment on attachment 8812531 [details]
Bug 1305486 - Enable v4 tables on nightly build. .

https://reviewboard.mozilla.org/r/94240/#review94416

Let's land this a day or two after bug 1315386 has landed on mozilla-central.

Comment 6

[Henry Chang [:hchang]]

The v4 tables caused non-local network access which leads to horrible try results [1]. We might need to override browser.safebrowsing.provider.google4.updateURL to dummy URL as [2] does.

Francois, do you have any other better suggestion? I'll do every bit [2] does otherwise.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=c225a271e6d8e76f30f4ca378a898e036ccad8ff&selectedJob=32120413
[2] https://dxr.mozilla.org/mozilla-central/search?q=browser.safebrowsing.provider.google.updateURL&redirect=false

Comment 7

[Henry Chang [:hchang]]

Comment on attachment 8812531 [details]
Bug 1305486 - Enable v4 tables on nightly build. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94240/diff/2-3/

Comment 8

[Thomas Nguyen (:tnguyen)]

There are some unconfirmed reports in bug 1321449, probably we need to wait for a while before landing this one.

Comment 9

[Henry Chang [:hchang]]

(In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #8)
> There are some unconfirmed reports in bug 1321449, probably we need to wait
> for a while before landing this one.

It seems irrelevant to v4 code. I tend to land no matter what but I'd rather wait
for Francois's call.

Comment 10

[François Marier [:francois]]

(In reply to Henry Chang [:henry][:hchang] from comment #6)
> The v4 tables caused non-local network access which leads to horrible try
> results [1]. We might need to override
> browser.safebrowsing.provider.google4.updateURL to dummy URL as [2] does.

That makes sense. Let's do that in a separate bug and land it now.

Comment 11

[François Marier [:francois]]

(In reply to Henry Chang [:henry][:hchang] from comment #9)
> (In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #8)
> > There are some unconfirmed reports in bug 1321449, probably we need to wait
> > for a while before landing this one.
> 
> It seems irrelevant to v4 code. I tend to land no matter what but I'd rather
> wait
> for Francois's call.

I'm tempted to agree with Thomas here. While it may not be related to V4, if we enable V4 updates, it may add more noise and make it more difficult to isolate the problems we're currently seeing

Comment 12

[Henry Chang [:hchang]]

Comment on attachment 8812531 [details]
Bug 1305486 - Enable v4 tables on nightly build. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94240/diff/3-4/

Comment 13

[Henry Chang [:hchang]]

Moved prefs changes to firefox.js. Francois, would you mind having a look again? Thanks :)

Comment 14

[François Marier [:francois]]

Comment on attachment 8812531 [details]
Bug 1305486 - Enable v4 tables on nightly build. .

https://reviewboard.mozilla.org/r/94240/#review104288

Looks good.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ee958f9deca
Enable v4 tables on nightly build. r=francois.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=67814144&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/7144e709f765

Comment 17

[Henry Chang [:hchang]]

sorry I overlooked this failure in the try result because it was marked as orange but not red :(

Comment 18

[Henry Chang [:hchang]]

Attachment: Bug1305486.patch

Comment 19

[Henry Chang [:hchang]]

Attachment: Bug1305486_no_v4_tests.patch

Comment 20

[Henry Chang [:hchang]]

I am explaining the try failure:

1) The reason I overlooked the try error is there are already a bunch of 
   'test_safe_browsing_initial_download.py' intermittent failure [1]

2) I revised the test case for v4 case but it still failed [2]. See attachment 8825692 [details] [diff] [review] (Bug1305486.patch)
   I am guessing it's due to the API key is not properly deployed on the try build machine instances.
   I've run another patch to reveal the API key to see if I guess correctly [3]

3) If my guess is true, I plan to land attachment 8825695 [details] [diff] [review] (Bug1305486_no_v4_tests.patch)
   to avoid v4 cases and file a follow-up bug to ask if CI machines can/need to be deployed
   valid google api key.

[1]
1282056 Intermittent test_safe_browsing_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download | TimeoutException: Timed out after 30.0 seconds with message: Safe Browsing File: ^goog-badbinurl-shavar.cache$ not
1309764 Intermittent test_safe_browsing_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download | TimeoutException: Timed out after 60.0 seconds with message: Not all safebrowsing files have been downloaded
1305435 Intermittent test_safe_browsing_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download | OSError: [Errno 1] Operation not permitted
1304781 Intermittent test_safe_browsing_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download | TimeoutException: Timed out after 30.1 seconds with message: Safe Browsing File: ^goog-badbinurl-shavar.pset$ not found!
1308368 Intermittent test_safe_browsing_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download | AssertionError: Items in the first set but not the second: 

[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a6c5cc5339322c7af6797a764e85235b9d79cd6&selectedJob=67911845
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c27f739217766647262af6449ca0cab1ff067bf&selectedJob=67928539

Comment 21

[Henry Chang [:hchang]]

Attachment: Bug1305486_no_v4_tests_with_followup_bug_number.patch

Comment 22

[Henry Chang [:hchang]]

The answer is out! 

At least the try server doesn't have google api key. I have filed Bug 1330253 to keep
track of this issue and the current question is: can we just not test v4 cases
in test_safe_browsing_initial_download.py? If the answer is yes, the patch I'd to 
land is attachment 8825706 [details] [diff] [review] (Bug1305486_no_v4_tests_with_followup_bug_number.patch).

Comment 23

[Henry Chang [:hchang]]

Attachment: Bug1305489_no_v4_tests_with_followup_bug_number.patch

Comment 24

[Henry Chang [:hchang]]

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

Comment 25

[François Marier [:francois]]

Comment on attachment 8825815 [details] [diff] [review]
Bug1305489_no_v4_tests_with_followup_bug_number.patch

Review of attachment 8825815 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Henry Chang [:henry][:hchang] from comment #22)
> can we just not test v4 cases in test_safe_browsing_initial_download.py?

The long-term answer is no. We do want that test to continue to run after we switch to V4.

However, I don't think it should block the roll-out to Nightly, so let's land your patch and follow-up in bug 1330253.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py
@@ +32,5 @@
>      ]
>  
>      prefs_provider_update_time = {
>          # Force an immediate download of the safebrowsing files
> +        # Bug 1330253 - Uncomment the following line until we deploy google api key

That should read "Comment" instead of "uncomment".

However, I would suggest a simpler comment: "Leave the next line disabled until we..."

@@ +96,5 @@
>              Wait(self.marionette, timeout=60).until(
>                  check_downloaded, message='Not all safebrowsing files have been downloaded')
>          finally:
> +            self.assertSetEqual(self.safebrowsing_v2_files, set(os.listdir(self.safebrowsing_path)))
> +            # Bug 1330253 - Uncomment the following line until we deploy google api key

ditto

Comment 26

[Henry Chang [:hchang]]

Attachment: Bug1305486_checkin-needed.patch

Comment 27

[Henry Chang [:hchang]]

Running try all again (hopefully it's the last one...)

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

Patch Bug1305486_checkin-needed has been addressed the comment and fixed lint error.

Comment 28

[Henry Chang [:hchang]]

try looks good! Flagged checkin-needed.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca2fd69cb777
Enable v4 tables on nightly build. r=francois

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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