Permaorange in test_safe_browsing_initial_download.py when Gecko 55 merges to beta on 2017-06-12

RESOLVED FIXED in Firefox 55

Status

Testing
Firefox UI Tests
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: RyanVM, Assigned: dimi)

Tracking

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55+ fixed, firefox56 fixed)

Details

MozReview Requests

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

Attachments

(3 attachments)

(Reporter)

Description

11 months ago
[Tracking Requested - why for this release]: Permaorange after Monday's uplift

These are Tier 2, so not going to keep the trees closed after the uplift, but an incoming permafail nonetheless.

https://treeherder.mozilla.org/logviewer.html#?job_id=106007990&repo=try
Flags: needinfo?(hskupin)
For the safe browsing test I'm cc'ing Francois. Not sure what difference we currently have between Nightly and when it is alive on Beta. Maybe some lists are still prefed off?

[task 2017-06-10T01:10:07.530730Z] 01:10:07     INFO - TEST-UNEXPECTED-ERROR | test_safe_browsing_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download | TimeoutException: Timed out after 60.1 seconds with message: Not all safebrowsing files have been downloaded
[task 2017-06-10T01:10:07.530817Z] 01:10:07     INFO - Traceback (most recent call last):
[task 2017-06-10T01:10:07.530898Z] 01:10:07     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/testcases.py", line 157, in run
[task 2017-06-10T01:10:07.530946Z] 01:10:07     INFO -     testMethod()
[task 2017-06-10T01:10:07.531045Z] 01:10:07     INFO -   File "/home/worker/workspace/build/tests/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py", line 94, in test_safe_browsing_initial_download
[task 2017-06-10T01:10:07.531113Z] 01:10:07     INFO -     check_downloaded, message='Not all safebrowsing files have been downloaded')
Flags: needinfo?(hskupin) → needinfo?(francois)
For the toolbar test:

[task 2017-06-10T01:08:46.696214Z] 01:08:46     INFO - TEST-UNEXPECTED-FAIL | test_toolbars.py TestAutoCompleteResults.test_popup_elements | AssertionError: 9 != 10
[task 2017-06-10T01:08:46.696307Z] 01:08:46     INFO - Traceback (most recent call last):
[task 2017-06-10T01:08:46.697095Z] 01:08:46     INFO -   File "/home/worker/workspace/build/venv/local/lib/python2.7/site-packages/marionette_harness/marionette_test/testcases.py", line 157, in run
[task 2017-06-10T01:08:46.697166Z] 01:08:46     INFO -     testMethod()
[task 2017-06-10T01:08:46.697342Z] 01:08:46     INFO -   File "/home/worker/workspace/build/tests/firefox-ui/tests/puppeteer/test_toolbars.py", line 165, in test_popup_elements
[task 2017-06-10T01:08:46.697885Z] 01:08:46     INFO -     int(results.get_property('itemCount')))
[task 2017-06-10T01:08:46.698828Z] 01:08:46     INFO - TEST-INFO took 1024ms

The number of autocomplete items is different. Marco, will the search changes not hit beta, or is there something else which could cause a different number of results to be reported?
Flags: needinfo?(mak77)
(In reply to Henrik Skupin (:whimboo) from comment #2)
> For the toolbar test:
> 
> [task 2017-06-10T01:08:46.696214Z] 01:08:46     INFO - TEST-UNEXPECTED-FAIL
> | test_toolbars.py TestAutoCompleteResults.test_popup_elements |
> AssertionError: 9 != 10

Nope, after the uplift beta and nightly have the same code and features, and both still return a maximum of 10 results. So I can't explain a difference in the behavior, unless history/bookmarks contains less than 10 elements in beta. I think that we have less default bookmarks in Beta than in Nightly?
note that, this is comparing _matchCount (that is basically min(controller.matchCount, 10)) with the number of items in the richlistbox. Those can be different, because we hide and reuse richlistitems to avoid destroying and creating them, they are just hidden. I think that itemCount also accounts for richlistitems that are there but collapsed. maybe you should loop richlistbox.children and count the not-collapsed ones?
So here the test:

https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui/tests/puppeteer/test_toolbars.py?q=test_popup_elements&redirect_type=single#155

And the interesting TODO comment:

> # TODO: This test is not very robust because it relies on the history in the default profile.

Anyway.... we open the autocomplete and get `9` items via `visible_results`. And via `get_property('itemCount')` we get `10`. So the test would have to be changed to a lessEqual check, or we use a different property than `itemCount` which will return the visible entries, is available. Marco, do we have such one? If not I would go the route to change the assert to lessEqual.
(In reply to Marco Bonardo [::mak] from comment #4)
> also accounts for richlistitems that are there but collapsed. maybe you
> should loop richlistbox.children and count the not-collapsed ones?

If that is a proper solution, we could definitely do this here:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/browser/toolbars.py#348

Let me know and I will file a new bug to get this work done.
(In reply to Henrik Skupin (:whimboo) from comment #6)
> If that is a proper solution, we could definitely do this here:
> https://dxr.mozilla.org/mozilla-central/source/testing/marionette/puppeteer/
> firefox/firefox_puppeteer/ui/browser/toolbars.py#348
> 
> Let me know and I will file a new bug to get this work done.

if you can loop through .children and do a manual count for uncollapsed items, instead of using itemCount, it may work.
Flags: needinfo?(mak77)

Updated

11 months ago
Depends on: 1372205
Ryan, what I wonder is why those failures are getting reported now just a few days before the merge. Weren't those visible already before by the other merges sheriffs did?
Flags: needinfo?(ryanvm)
(Reporter)

Comment 9

11 months ago
Phil doesn't look at Tier 2 jobs.
Flags: needinfo?(ryanvm)
(Reporter)

Comment 10

11 months ago
The test_safe_browsing_initial_download.py looks to have been a red herring (maybe an issue with running on Try in general?) - they're not happening on Beta post-uplift. The test_toolbars.py are still there.
(Reporter)

Comment 11

11 months ago
Turns out the test_safe_browsing_initial_download.py failures are still there, just intermittent instead of permanent.
Created attachment 8876773 [details]
safebrowsing debug log

Here the log output from the safebrowsing component. Maybe that helps.
The current failure for one of my try builds is:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f42e7829e8111ef30afc2358c9669707d5e2718&selectedJob=106399184

AssertionError: 'goog-phish-shavar.sbstore' not found in ['allow-flashallow-digest256.pset', 'allow-flashallow-digest256.sbstore', 'base-track-digest256.pset', 'base-track-digest256.sbstore', 'block-flash-digest256.pset', 'block-flash-digest256.sbstore', 'block-flashsubdoc-digest256.pset', 'block-flashsubdoc-digest256.sbstore', 'except-flash-dige [log…]
(Reporter)

Comment 14

11 months ago
OK, the safebrowsing failures are not intermittent, but they only happen on opt builds. Debug and ASAN builds are fine. Which makes sense because they're disabled there. So yeah, permafail they are.
Looks like there is bug 1309764 for the timeout already. Maybe we should use that for fixing the issue, and keep this bug open for tracking.
Depends on: 1309764
tracking-firefox55: ? → +
(In reply to Henrik Skupin (:whimboo) from comment #13)
> The current failure for one of my try builds is:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2f42e7829e8111ef30afc2358c9669707d5e2718&selectedJob=1
> 06399184
> 
> AssertionError: 'goog-phish-shavar.sbstore' not found in
> ['allow-flashallow-digest256.pset', 'allow-flashallow-digest256.sbstore',
> 'base-track-digest256.pset', 'base-track-digest256.sbstore',
> 'block-flash-digest256.pset', 'block-flash-digest256.sbstore',
> 'block-flashsubdoc-digest256.pset', 'block-flashsubdoc-digest256.sbstore',
> 'except-flash-dige [log…]

The problem in this one is that the updateURL is empty for the google provider (where the goog-phish-shavar) list comes from:

https://treeherder.mozilla.org/logviewer.html#?job_id=106399184&repo=try&lineNumber=11899

so we never even attempt to download the list.

I'm not sure why that's empty though. It's properly set in Ryan's try job from comment 0:

https://treeherder.mozilla.org/logviewer.html#?job_id=106007990&repo=try&lineNumber=12160

and the update goes through successfully:

https://treeherder.mozilla.org/logviewer.html#?job_id=106007990&repo=try&lineNumber=12716
Flags: needinfo?(francois)
(In reply to François Marier [:francois] from comment #16)
> I'm not sure why that's empty though.

Maybe the Google API key isn't set in that build? (you can check that in about:support)

In bug 1336915, we added a check to disable updates when the key is missing.
Given that both google and mozilla updates work fine:

https://treeherder.mozilla.org/logviewer.html#?job_id=106007990&repo=try&lineNumber=12716
https://treeherder.mozilla.org/logviewer.html#?job_id=106007990&repo=try&lineNumber=12750

it's not related to these providers. What's happening instead is that we are
timing out waiting for google4 to return. However, we don't have any lists
from that provider so we shouldn't be waiting for it.

It worked on Nightly 55 because Nightly has all of the providers enabled.
It worked on Beta 54 because that was before the check for google4 lists was
re-enabled in bug 1330253.

So what we need to do, is modify the waiting condition that Henrik added in bug 1304983:

- Always enable browser.safebrowsing.provider.mozilla.
- Only enable provider.google if safebrowsing_v2_files includes at least one list that starts with "goog-".
- Only enable provider.google4 if safebrowsing_v4_files includes at least one list that starts with "goog-".
Francois, thank you for the investigation! Who can fix it up? Can you or Henry do it?
Flags: needinfo?(francois)
I will check with Dimi, Henry and Thomas because I won't have time to do it until next week.

The fix should be fairly trivial if my proposal from comment 18 works, but it will need to be tested on try for beta 55 and nightly 56.
Flags: needinfo?(francois)
(Assignee)

Updated

10 months ago
Assignee: nobody → dlee
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 23

10 months ago
I have test this patch on both nightly and beta 55 on try.
(Assignee)

Comment 24

10 months ago
Comment on attachment 8878367 [details]
Bug 1371923 - Only check safebrowsing files when they are in preference in test_safe_browsing_initial_download.py.

cancel the review because I uploaded the wrong patch
Attachment #8878367 - Flags: review?(francois)
Comment hidden (mozreview-request)
Does that problem also explain the test failures on try for my push?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c858c0d4319&selectedJob=107700226
Summary: Permaorange in test_toolbars.py and test_safe_browsing_initial_download.py when Gecko 55 merges to beta on 2017-06-12 → Permaorange in test_safe_browsing_initial_download.py when Gecko 55 merges to beta on 2017-06-12
(In reply to Henrik Skupin (:whimboo) from comment #26)
> Does that problem also explain the test failures on try for my push?
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7c858c0d4319&selectedJob=107700226

That's a different problem. The update for the V4 lists fails with a 400:

https://treeherder.mozilla.org/logviewer.html#?job_id=107700226&repo=try&lineNumber=12845

Was that just a normal Try push? Try should have the correct API key set...
(In reply to François Marier [:francois] from comment #27)
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=107700226&repo=try&lineNumber=12845
> 
> Was that just a normal Try push? Try should have the correct API key set...

It turned out that this only happens for artifact builds. That is what all those builds are. Another try push without the --artifact flag made it work. Sounds like I should file a bug for that.

Comment 29

10 months ago
mozreview-review
Comment on attachment 8878367 [details]
Bug 1371923 - Only check safebrowsing files when they are in preference in test_safe_browsing_initial_download.py.

https://reviewboard.mozilla.org/r/149716/#review154628

Did you forget to go through the list of files looking for "goog-"?

That's the most reliable way to check whether or not the files should be present.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:37
(Diff revision 2)
>      ]
>  
>      prefs_provider_update_time = {
>          # Force an immediate download of the safebrowsing files
> -        'browser.safebrowsing.provider.google4.nextupdatetime': 1,
> -        'browser.safebrowsing.provider.google.nextupdatetime': 1,
> +        # 'google' and ''google4' will be added in setUp.
> +        # 'browser.safebrowsing.provider.google4.nextupdatetime': 1,

We may as well remove the two `browser.safebrowsing.provider.google*` lines.

::: testing/firefox-ui/tests/functional/security/test_safe_browsing_initial_download.py:72
(Diff revision 2)
>  
>      def setUp(self):
>          super(TestSafeBrowsingInitialDownload, self).setUp()
>  
> +        self.safebrowsing_v2_files = self.get_safebrowsing_files(False)
> +        if self.safebrowsing_v2_files:

That check is insufficient because we will always have V2 files (tracking protection).
Attachment #8878367 - Flags: review?(francois) → review-
(Assignee)

Comment 30

10 months ago
(In reply to François Marier [:francois] from comment #29)
> Comment on attachment 8878367 [details]
> Bug 1371923 - Only check safebrowsing files when they are in preference in
> test_safe_browsing_initial_download.py.
> 
> https://reviewboard.mozilla.org/r/149716/#review154628
> 
> Did you forget to go through the list of files looking for "goog-"?
> 
> That's the most reliable way to check whether or not the files should be
> present.

Yes, you are right, I forgot that.
I'll fix it in next patch.
Comment hidden (mozreview-request)
(In reply to Henrik Skupin (:whimboo) from comment #28)
> (In reply to François Marier [:francois] from comment #27)
> > https://treeherder.mozilla.org/logviewer.
> > html#?job_id=107700226&repo=try&lineNumber=12845
> > 
> > Was that just a normal Try push? Try should have the correct API key set...
> 
> It turned out that this only happens for artifact builds. That is what all
> those builds are. Another try push without the --artifact flag made it work.
> Sounds like I should file a bug for that.

Filed bug 1374268 for that.

Comment 34

10 months ago
mozreview-review
Comment on attachment 8878367 [details]
Bug 1371923 - Only check safebrowsing files when they are in preference in test_safe_browsing_initial_download.py.

https://reviewboard.mozilla.org/r/149716/#review155232

Thanks Dimi!

::: commit-message-79cdd:3
(Diff revision 3)
> +Bug 1371923 - Only check safebrowsing files when they are in preference in test_safe_browsing_initial_download.py. r?francois
> +
> +In test_safe_browsing_initial_download.py, we will wait for safebrowsing files even they are not included in preference.

Suggested improvement to clarify the effect of the bug:

"Ensure that we only wait for the providers that are enabled (i.e. have active lists) to avoid waiting forever and timing out."

::: commit-message-79cdd:4
(Diff revision 3)
> +Bug 1371923 - Only check safebrowsing files when they are in preference in test_safe_browsing_initial_download.py. r?francois
> +
> +In test_safe_browsing_initial_download.py, we will wait for safebrowsing files even they are not included in preference.
> +The fix in this patch is:

"In order to check whether or not the google and google4 providers are enabled, we look for lists that start with "goog" in both the V2 files and the V4 files."

This could replace the two bullet points too.
Attachment #8878367 - Flags: review?(francois) → review+
Duplicate of this bug: 1368542
Duplicate of this bug: 1309764
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 39

10 months ago
Pushed by dlee@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92846886fda2
Only check safebrowsing files when they are in preference in test_safe_browsing_initial_download.py. r=francois
(Assignee)

Comment 40

10 months ago
Created attachment 8879349 [details] [diff] [review]
Patch for beta

Approval Request Comment
[Feature/Bug causing the regression]: This is not a regression, it is because we enable Safebrowsing V4 table in nightly and add testcase for that (Bug 1305486) ), but after merging to beta this testcase will fail because there is no V4 tables.

[User impact if declined]: No, this patch only fixes testcase
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: This patch only fixes testcase
[String changes made/needed]: No
Attachment #8879349 - Flags: approval-mozilla-beta?
(Reporter)

Comment 41

10 months ago
Comment on attachment 8879349 [details] [diff] [review]
Patch for beta

test-only changes don't need approval, thanks for fixing this!
Attachment #8879349 - Flags: approval-mozilla-beta?
(Reporter)

Updated

10 months ago
Whiteboard: [checkin-needed-beta]

Comment 42

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/92846886fda2
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Reporter)

Comment 43

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d706df28a15b
status-firefox55: affected → fixed
Whiteboard: [checkin-needed-beta]

Comment 44

10 months ago
7 failures in 892 pushes (0.008 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-beta: 7

Platform breakdown:
* linux64-nightly: 2
* linux64-devedition: 2
* linux32-devedition: 2
* linux32-nightly: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1371923&startday=2017-06-19&endday=2017-06-25&tree=all
You need to log in before you can comment on or make changes to this bug.