Closed Bug 1338638 Opened 7 years ago Closed 7 years ago

Intermittent test_safe_browsing_initial_download.py TestSafeBrowsingInitialDownload.test_safe_browsing_initial_download | AssertionError: 'mozplugin-block-digest256.sbstore' not found in ['test-phish-simple.sbstore', 'test-unwanted-simple.sbstore',

Categories

(Toolkit :: Safe Browsing, defect, P1)

Product:
Toolkit
Component:
Safe Browsing
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: hchang)

References

Details

(Keywords: intermittent-failure, regression, Whiteboard: [stockwell fixed])

Attachments

(1 file)

Bug 1338638 - Fix race condition for DBService APIs to avoid long delayed initial download.
59 bytes, text/x-review-board-request
francois
: review+
Details 
Francois, can you please have a look? Something seems to be wrong here.
Flags: needinfo?(francois)
Whiteboard: [stockwell needswork]
Thank you Geoff for checking. Henry, can you please have a look? Thanks.
Flags: needinfo?(francois) → needinfo?(hchang)
Keywords: regression
Assignee: nobody → hchang
Flags: needinfo?(hchang)
Attachment #8856898 - Flags: review?(francois)
Blocks: 1351380
See Also: 1351380 → 
Comment on attachment 8856898 [details]
Bug 1338638 - Fix race condition for DBService APIs to avoid long delayed initial download.

https://reviewboard.mozilla.org/r/128820/#review131530

That solution looks great. I only have a few nits.

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.h:87
(Diff revision 1)
>    nsCOMPtr<nsIUrlClassifierDBService> mDBService;
> -  nsCOMPtr<nsITimer> mTimer;
>  
> +  // In v2, a update response might contain redirection and this
> +  // timer is for fetching the redirected update.
> +  nsCOMPtr<nsITimer> mFetchNextUpdateTimer;

The naming is a little confusing now that we have two timers.

How about `mFetchIndirectUpdatesTimer` for this one?

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:290
(Diff revision 1)
>      request->mUrl = aUpdateUrl;
>      request->mSuccessCallback = aSuccessCallback;
>      request->mUpdateErrorCallback = aUpdateErrorCallback;
>      request->mDownloadErrorCallback = aDownloadErrorCallback;
> +
> +    // We cannot guarantee DBService is busy on processing request

Based on your commit message, I would suggest this explanation:

"We cannot guarantee that we will be notified when DBService is done processing the current update, so we fire a retry timer on our own."

::: toolkit/components/url-classifier/nsUrlClassifierStreamUpdater.cpp:878
(Diff revision 1)
> -  // Start the update process up again.
> +    // Start the update process up again.
> -  FetchNext();
> +    FetchNext();
> +    return NS_OK;
> +  }
>  
> +  NS_WARNING("A timer is fired from nowhere.");

Maybe that should be an assert instead so that it fails on debug builds in try?
Attachment #8856898 - Flags: review?(francois) → review-
Component: Firefox UI Tests → Safe Browsing
Product: Testing → Toolkit
QA Contact: hskupin
Version: Version 3 → unspecified
Comment on attachment 8856898 [details]
Bug 1338638 - Fix race condition for DBService APIs to avoid long delayed initial download.

https://reviewboard.mozilla.org/r/128820/#review132174
Attachment #8856898 - Flags: review?(francois) → review+
Status: NEW → ASSIGNED
Priority: -- → P1
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/17180b888f7c
Fix race condition for DBService APIs to avoid long delayed initial download. r=francois
https://hg.mozilla.org/mozilla-central/rev/17180b888f7c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Whiteboard: [stockwell needswork] → [stockwell fixed]
These failures affect 54 too. Should we consider uplifting?
status-firefox54: unaffected → affected
Flags: needinfo?(hchang)
The fix is fairly simple so uplifting around should be safe. I'll do it. Thanks!
Flags: needinfo?(hchang)
Oops. After checking the 54 code base, the root cause of this issue (Bug 1339050) is not there.

Ryan, could you suggest how you conclude that? I checked 

https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora

and didn't see relevant failures :(
54 is on Beta now, for one thing :). Maybe the failures are similar but different then?

https://treeherder.mozilla.org/logviewer.html#?job_id=93849205&repo=mozilla-beta
Flags: needinfo?(hchang)
As far as I can tell, that's a similar failure and we do have one intermittent bug
for that test case. So the failure you indicated is only appeared in beta but not 
in aurora?
Flags: needinfo?(hchang)
Aurora doesn't exist anymore.
After discussing with Henry, the root cause of this intermittent error in 54 is different from 55 Nightly. Given the root cause is different, mark 54 won't fix.
status-firefox54: affected → wontfix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: