Closed Bug 1305486 Opened 8 years ago Closed 7 years ago

Enable V4 update by default on Nightly only

Categories

(Toolkit :: Safe Browsing, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: francois, Assigned: hchang)

References

Details

(Whiteboard: #sbv4-m4)

Attachments

(2 files, 5 obsolete files)

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.
Depends on: 1305780
Depends on: 1305801
No longer depends on: 1305780
No longer depends on: 1305801
Depends on: 1305801
Whiteboard: #sbv4-m1 → #sbv4-m2
Depends on: 1305780
Depends on: 1310060
Depends on: 1310142
Depends on: 1315097
Attachment #8797026 - Attachment is obsolete: true
Depends on: 1315386
No longer depends on: 1310060
Assignee: francois → hchang
Attachment #8812531 - Flags: review?(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.
Attachment #8812531 - Flags: review?(francois) → review-
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.
Attachment #8812531 - Flags: review?(francois) → review+
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
Flags: needinfo?(francois)
There are some unconfirmed reports in bug 1321449, probably we need to wait for a while before landing this one.
(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.
(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.
Flags: needinfo?(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
Whiteboard: #sbv4-m2 → #sbv4-m4
Depends on: 1324820
Moved prefs changes to firefox.js. Francois, would you mind having a look again? Thanks :)
Flags: needinfo?(francois)
Comment on attachment 8812531 [details]
Bug 1305486 - Enable v4 tables on nightly build. .

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

Looks good.
Flags: needinfo?(francois)
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ee958f9deca
Enable v4 tables on nightly build. r=francois.
sorry I overlooked this failure in the try result because it was marked as orange but not red :(
Flags: needinfo?(hchang)
Attached patch Bug1305486.patch (obsolete) — Splinter Review
Attached patch Bug1305486_no_v4_tests.patch (obsolete) — Splinter Review
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
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).
Flags: needinfo?(francois)
Attachment #8825706 - Attachment is obsolete: true
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
Attachment #8825815 - Flags: review+
Attachment #8825692 - Attachment is obsolete: true
Attachment #8825695 - Attachment is obsolete: true
Attachment #8825815 - Attachment is obsolete: true
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.
try looks good! Flagged checkin-needed.
Flags: needinfo?(francois)
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ca2fd69cb777
Enable v4 tables on nightly build. r=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ca2fd69cb777
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
No longer depends on: 1324820
You need to log in before you can comment on or make changes to this bug.