Enable V4 update by default on Nightly only

RESOLVED FIXED in Firefox 53

Status

()

P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: francois, Assigned: hchang)

Tracking

unspecified
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: #sbv4-m4)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

2 years ago
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.

Updated

2 years ago
Depends on: 1305780

Updated

2 years ago
Depends on: 1305801

Updated

2 years ago
No longer depends on: 1305780
(Reporter)

Updated

2 years ago
No longer depends on: 1305801
(Assignee)

Comment 1

2 years ago
Created attachment 8797026 [details] [diff] [review]
0001-Bug-1305486-Add-googpub-phish-proto-goog-malware-pro.patch

Updated

2 years ago
Depends on: 1305801

Updated

2 years ago
Whiteboard: #sbv4-m1 → #sbv4-m2
(Reporter)

Updated

2 years ago
Depends on: 1305780
(Reporter)

Updated

2 years ago
Depends on: 1310060
(Reporter)

Updated

2 years ago
Depends on: 1310142
(Reporter)

Updated

2 years ago
Depends on: 1315097
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8797026 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Depends on: 1315386
No longer depends on: 1310060
(Assignee)

Updated

2 years ago
Assignee: francois → hchang
(Assignee)

Updated

2 years ago
Attachment #8812531 - Flags: review?(francois)
(Reporter)

Comment 3

2 years ago
mozreview-review
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 hidden (mozreview-request)
(Reporter)

Comment 5

2 years ago
mozreview-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+
(Assignee)

Comment 6

2 years ago
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)
Comment hidden (mozreview-request)
There are some unconfirmed reports in bug 1321449, probably we need to wait for a while before landing this one.
(Assignee)

Comment 9

2 years ago
(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.
(Reporter)

Comment 10

2 years ago
(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)
(Reporter)

Comment 11

2 years ago
(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
(Assignee)

Updated

2 years ago
Whiteboard: #sbv4-m2 → #sbv4-m4

Updated

2 years ago
Depends on: 1324820
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
Moved prefs changes to firefox.js. Francois, would you mind having a look again? Thanks :)
Flags: needinfo?(francois)
(Reporter)

Comment 14

2 years ago
mozreview-review
Comment on attachment 8812531 [details]
Bug 1305486 - Enable v4 tables on nightly build. .

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

Looks good.
(Reporter)

Updated

2 years ago
Flags: needinfo?(francois)

Comment 15

2 years ago
Pushed by hchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ee958f9deca
Enable v4 tables on nightly build. r=francois.
(Assignee)

Comment 17

2 years ago
sorry I overlooked this failure in the try result because it was marked as orange but not red :(
Flags: needinfo?(hchang)
(Assignee)

Comment 19

2 years ago
Created attachment 8825695 [details] [diff] [review]
Bug1305486_no_v4_tests.patch
(Assignee)

Comment 20

2 years ago
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
(Assignee)

Comment 21

2 years ago
Created attachment 8825706 [details] [diff] [review]
Bug1305486_no_v4_tests_with_followup_bug_number.patch
(Assignee)

Comment 22

2 years ago
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)
(Assignee)

Comment 23

2 years ago
Created attachment 8825815 [details] [diff] [review]
Bug1305489_no_v4_tests_with_followup_bug_number.patch
(Assignee)

Updated

2 years ago
Attachment #8825706 - Attachment is obsolete: true
(Reporter)

Comment 25

2 years ago
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+
(Assignee)

Comment 26

2 years ago
Created attachment 8826071 [details] [diff] [review]
Bug1305486_checkin-needed.patch
Attachment #8825692 - Attachment is obsolete: true
Attachment #8825695 - Attachment is obsolete: true
Attachment #8825815 - Attachment is obsolete: true
(Assignee)

Comment 27

2 years ago
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.
(Assignee)

Comment 28

2 years ago
try looks good! Flagged checkin-needed.
Flags: needinfo?(francois)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 29

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ca2fd69cb777
Enable v4 tables on nightly build. r=francois
Keywords: checkin-needed

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ca2fd69cb777
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Updated

2 years ago
No longer depends on: 1324820
You need to log in before you can comment on or make changes to this bug.