Closed Bug 1289028 Opened 3 years ago Closed 3 years ago

Intermittent toolkit/components/url-classifier/tests/mochitest/test_gethash.html | Test timed out.

Categories

(Toolkit :: Safe Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

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

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Assignee: nobody → dlee
Status: NEW → ASSIGNED
I found this bug happens because we will call beginUpdate when Safebrowsing.jsm init[1] and when testcase is start to run, we may also call beginUpdate. If the timing is bad, second beginUpdate may fail because of the first one is still running[2].

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/SafeBrowsing.jsm#375
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp#1505
Attachment #8775482 - Flags: feedback?(hchang)
I don't really have a good way to make sure testcase call beginUpdate after Safebrowsing.jsm.
So use retry in this patch
Comment on attachment 8775482 [details]
Bug 1289028 - Retry when url-classifier test_gethash.html hits an exception.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67656/diff/1-2/
Attachment #8775482 - Attachment description: Bug 1289028 - Intermittent toolkit/components/url-classifier/tests/mochitest/test_gethash.html | Test timed out. f?hchang → Bug 1289028 - Intermittent toolkit/components/url-classifier/tests/mochitest/test_gethash.html | Test timed out.
Attachment #8775482 - Flags: feedback?(hchang)
Comment on attachment 8775482 [details]
Bug 1289028 - Retry when url-classifier test_gethash.html hits an exception.

https://reviewboard.mozilla.org/r/67656/#review64762

It mostly looks good to me and I've only got some suggestions. Thanks!

::: toolkit/components/url-classifier/tests/mochitest/classifierCommon.js:42
(Diff revision 2)
>  
>    let dbService = Cc["@mozilla.org/url-classifier/dbservice;1"]
>                    .getService(Ci.nsIUrlClassifierDBService);
>  
> +  try {
> -  dbService.beginUpdate(listener, "test-malware-simple,test-unwanted-simple", "");
> +    dbService.beginUpdate(listener, "test-malware-simple,test-unwanted-simple", "");

If beginUpdate is the only function which may throw exception, maybe we can try-catch this function.

::: toolkit/components/url-classifier/tests/mochitest/classifierCommon.js:50
(Diff revision 2)
> -  dbService.finishStream();
> +    dbService.finishStream();
> -  dbService.finishUpdate();
> +    dbService.finishUpdate();
> +  } catch(e) {
> +    // beginUpdate may fail if there's an existing update in progress
> +    updateRetry++;
> +    if (updateRetry <= MAX_UPDATE_RETRY) {

I would suggest just to retry until timeout if there's no other concern.
Attachment #8775482 - Flags: review+
Comment on attachment 8775482 [details]
Bug 1289028 - Retry when url-classifier test_gethash.html hits an exception.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67656/diff/2-3/
Attachment #8775482 - Flags: review+ → review?(francois)
Per Dimi. The patch of this bug would also resolve the Treeherder failure of bug 1254766.
See Also: → 1254766
Blocks: 1254766
See Also: 1254766
Comment on attachment 8775482 [details]
Bug 1289028 - Retry when url-classifier test_gethash.html hits an exception.

https://reviewboard.mozilla.org/r/67656/#review64896

Looks good to me, but I would suggest a commit message which describes your fix:

    Bug 1289028 - Retry when url-classifier test_gethash.html hits an exception. r=francois
Attachment #8775482 - Flags: review?(francois) → review+
Comment on attachment 8775482 [details]
Bug 1289028 - Retry when url-classifier test_gethash.html hits an exception.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67656/diff/3-4/
Attachment #8775482 - Attachment description: Bug 1289028 - Intermittent toolkit/components/url-classifier/tests/mochitest/test_gethash.html | Test timed out. → Bug 1289028 - Retry when url-classifier test_gethash.html hits an exception.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/0ed2256fd1fd
Retry when url-classifier test_gethash.html hits an exception. r=francois
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ed2256fd1fd
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Duplicate of this bug: 1276042
Duplicate of this bug: 1199778
mochitest.ini still shows this test as being skipped on Linux debug due to bug 1199778. Did you mean to re-enable it as part of landing this fix?
Flags: needinfo?(dlee)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> mochitest.ini still shows this test as being skipped on Linux debug due to
> bug 1199778. Did you mean to re-enable it as part of landing this fix?

Yes, it could be re-enabled, do you want me to add a fix for this?
Flags: needinfo?(dlee)
Probably worth filing a new bug for it at this point and running it through Try.
You need to log in before you can comment on or make changes to this bug.