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

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Treeherder Bug Filer]

Filed by: tomcat [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=32507520&repo=mozilla-inbound

https://queue.taskcluster.net/v1/task/P3iem8P7RxSD5rRyyz7XxQ/runs/0/artifacts/public%2Flogs%2Flive_backing.log

Comment 1

[Dimi Lee [:dimi]]

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

Comment 2

[Dimi Lee [:dimi]]

Attachment: Bug 1289028 - Retry when url-classifier test_gethash.html hits an exception.

Review commit: https://reviewboard.mozilla.org/r/67656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67656/

Comment 3

[Dimi Lee [:dimi]]

I don't really have a good way to make sure testcase call beginUpdate after Safebrowsing.jsm.
So use retry in this patch

Comment 4

[Dimi Lee [:dimi]]

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/

Comment 5

[Henry Chang [: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.

Comment 6

[Dimi Lee [:dimi]]

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/

Comment 7

[Ethan Tseng [:ethan]]

Per Dimi. The patch of this bug would also resolve the Treeherder failure of bug 1254766.

Comment 8

[François Marier [:francois]]

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

Comment 9

[Dimi Lee [:dimi]]

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/

Comment 10

[Dimi Lee [:dimi]]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=516158950ec7

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/0ed2256fd1fd
Retry when url-classifier test_gethash.html hits an exception. r=francois

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ed2256fd1fd

Comment 13

[Intermittent Failures Robot]

15 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-inbound: 8
* autoland: 5
* fx-team: 2

Platform breakdown:
* linux64: 5
* android-4-3-armv7-api15: 3
* windows8-64: 2
* windows7-32: 2
* osx-10-10: 2
* linux32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1289028&startday=2016-07-25&endday=2016-07-31&tree=all

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 17

[Dimi Lee [:dimi]]

(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?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Probably worth filing a new bug for it at this point and running it through Try.