934207 - Add browser_thumbnails_background_crash.js to browser.ini

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Looks like the test is run when running all of the thumbnail tests but it's impossible to run it in single mode.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0005-Bug-934207-Add-browser_thumbnails_background_crash.j.patch

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 826400 [details] [diff] [review]
0005-Bug-934207-Add-browser_thumbnails_background_crash.j.patch

Review of attachment 826400 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/thumbnails/test/browser.ini
@@ +11,5 @@
>  
>  [browser_thumbnails_background.js]
>  # Too many intermittent failures (bug 931889)
>  skip-if = os == "linux"
> +[browser_thumbnails_background_crash.js]

This will need |run-if = crashreporter| and Makefile.in should be removed (as these are the last 2 there) - see bug 932147 for where I'm doing the same in a different dir.  r=me for that, but I think it's probably best to upload a new patch with that, carry my r+ forward and request review from jhammel for the removal of the Makefile (r+ is pending from him in bug 932147, otherwise I'd just say "go for it" :)

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-934207-Add-browser_thumbnails_background_crash.j.patch

(In reply to Mark Hammond [:markh] from comment #2)
> This will need |run-if = crashreporter|

Does it? I build without the crash reporter locally and it runs just fine. The test checks whether the crash reporter is available.

> and Makefile.in should be removed

Ah, thanks. That's where the test was. Removed Makefile.in and asking jhammel for review as well.

Comment 4

[Jeff Hammel]

Comment on attachment 826681 [details] [diff] [review]
0001-Bug-934207-Add-browser_thumbnails_background_crash.j.patch

Review of attachment 826681 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm;

Comment 5

[Mark Hammond [:markh] [:mhammond]]

(In reply to Tim Taubert [:ttaubert] from comment #3)
> Created attachment 826681 [details] [diff] [review]
> 0001-Bug-934207-Add-browser_thumbnails_background_crash.j.patch
> 
> (In reply to Mark Hammond [:markh] from comment #2)
> > This will need |run-if = crashreporter|
> 
> Does it? I build without the crash reporter locally and it runs just fine.
> The test checks whether the crash reporter is available.

We explicitly added that condition to the Makefile in bug 919438 - I really can't recall much history (ie, why it was a problem on ASAN) other than it failed - Drew, can you remember?

Comment 6

[Drew Willcoxon :adw]

Christian mentioned it in bug 915384 comment 77.  Christian, do you know whether that's still accurate, given the conversation in this bug (see comment 3, comment 5)?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

FTR, /browser/base/content/test/social/browser_social_workercrash.js does the same |if ('nsICrashReporter' in Ci)| check and it's not using |run-if = crashreporter|.

Comment 8

[Christian Holler (:decoder)]

If you modified this test since bug 919438 to properly check for the crash reporter on its own, then it might not be required anymore to check it in browser.ini. If you want to make sure, just push it to try with linux-asan and you'll see if it fails the ASan builds. If that is green, then you can just go ahead and omit the run-if = crashreporter stuff.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://tbpl.mozilla.org/?tree=Try&rev=4242a3674e18

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 826681 [details] [diff] [review]
0001-Bug-934207-Add-browser_thumbnails_background_crash.j.patch

https://hg.mozilla.org/integration/fx-team/rev/c80b984d7c5c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c80b984d7c5c

Comment 12

[Ada [:adalucinet]]

Is there any help needed for manual testing?

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

No, seems to be running fine on our test infrastructure. Thanks!