1368143 - Intermittent browser/base/content/test/siteIdentity/browser_bug902156.js | Expected state for activeBlocked matches UI state - Got true, expected false

Status: RESOLVED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Treeherder Bug Filer]

Filed by: rvandermeulen [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=102362500&repo=autoland

https://archive.mozilla.org/pub/firefox/tinderbox-builds/autoland-macosx64-debug/autoland_yosemite_r7-debug_test-mochitest-e10s-browser-chrome-7-bm106-tests1-macosx-build1122.txt.gz

Comment 1

[Intermittent Failures Robot]

2 failures in 718 pushes (0.003 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 2

Platform breakdown:
* osx-10-10: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1368143&startday=2017-06-26&endday=2017-07-02&tree=all

Comment 2

[Intermittent Failures Robot]

3 failures in 656 pushes (0.005 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 2
* autoland: 1

Platform breakdown:
* osx-cross: 2
* osx-10-10: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1368143&startday=2017-07-03&endday=2017-07-09&tree=all

Comment 3

[Intermittent Failures Robot]

4 failures in 720 pushes (0.006 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* pine: 2
* autoland: 2

Platform breakdown:
* osx-cross: 2
* osx-10-10: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1368143&startday=2017-07-10&endday=2017-07-16&tree=all

Comment 4

[Intermittent Failures Robot]

32 failures in 822 pushes (0.039 failures/push) were associated with this bug in the last 7 days.   

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. ** 

Repository breakdown:
* mozilla-inbound: 12
* autoland: 8
* try: 7
* mozilla-central: 4
* cedar: 1

Platform breakdown:
* osx-10-10: 31
* macosx64-nightly: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1368143&startday=2017-07-17&endday=2017-07-23&tree=all

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

:johann, this test is failing often on osx, is it possibly you could take a look at fixing this sometime in the next week or two?

Comment 6

[Intermittent Failures Robot]

39 failures in 1008 pushes (0.039 failures/push) were associated with this bug in the last 7 days.   

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. ** 

Repository breakdown:
* mozilla-inbound: 21
* autoland: 14
* mozilla-central: 2
* try: 1
* oak: 1

Platform breakdown:
* osx-10-10: 39

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1368143&startday=2017-07-24&endday=2017-07-30&tree=all

Comment 7

[Intermittent Failures Robot]

17 failures in 888 pushes (0.019 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 9
* autoland: 7
* mozilla-central: 1

Platform breakdown:
* osx-10-10: 17

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1368143&startday=2017-07-31&endday=2017-08-06&tree=all

Comment 8

[Johann Hofmann [:johannh]]

This smells like bug 1348549 again. Only this test is a lot bigger :/

I'd like to note that I'm not the author of either test, but I guess I can fix this one as well.

Comment 9

[Johann Hofmann [:johannh]]

Attachment: Bug 1368143 - Modernize browser_bug902156.js.

- Switches to add_task and async functions.
- Waits for the correct url to be loaded in BTU.browserLoaded.
- Gets rid of all the CPOWs because we don't like them.

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

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8894495 [details]
Bug 1368143 - Modernize browser_bug902156.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165664/diff/1-2/

Comment 11

[Intermittent Failures Robot]

16 failures in 901 pushes (0.018 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 11
* mozilla-inbound: 5

Platform breakdown:
* osx-10-10: 16

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1368143&startday=2017-08-07&endday=2017-08-13&tree=all

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8894495 [details]
Bug 1368143 - Modernize browser_bug902156.js.

https://reviewboard.mozilla.org/r/165664/#review172090

Thanks! Tentative r+, with a couple of questions. Sorry for the delay reviewing this!

::: browser/base/content/test/siteIdentity/browser_bug902156.js:52
(Diff revision 2)
> -  BrowserTestUtils.waitForCondition(
> +      await ContentTaskUtils.waitForCondition(
> -    () => content.document.getElementById("mctestdiv").innerHTML == expected,
> +        () => content.document.getElementById("mctestdiv").innerHTML == expected,
> -    "Error: Waited too long for mixed script to run in Test 1B").then(test1C);
> +        "Error: Waited too long for mixed script to run in Test 1");
> -}
>  
> -function test1C() {
> +      let actual = content.document.getElementById("mctestdiv").innerHTML;

What's the point of checking this again after the waitForCondition call?

Also, shall we increase the scope of the expected string variable and reuse it (the same string is used again a few times)?

::: browser/base/content/test/siteIdentity/browser_bug902156.js:67
(Diff revision 2)
> -function test1D() {
> -  // The Control Center button should appear but isMixedContentBlocked should be NOT true,
> +    // The Control Center button should appear but isMixedContentBlocked should be NOT true,
> -  // because our decision of disabling the mixed content blocker is persistent.
> +    // because our decision of disabling the mixed content blocker is persistent.
> -  assertMixedContentBlockingState(gTestBrowser, {activeLoaded: true, activeBlocked: false, passiveLoaded: false});
> -
> -  var actual = content.document.getElementById("mctestdiv").innerHTML;
> +    assertMixedContentBlockingState(browser, {activeLoaded: true, activeBlocked: false, passiveLoaded: false});
> +    await ContentTask.spawn(browser, null, function() {
> +      let actual = content.document.getElementById("mctestdiv").innerHTML;

I'm not sure I fully understand the exact order of events, but how come we don't need a waitForCondition here like above?

Comment 13

[Johann Hofmann [:johannh]]

Comment on attachment 8894495 [details]
Bug 1368143 - Modernize browser_bug902156.js.

https://reviewboard.mozilla.org/r/165664/#review172090

> What's the point of checking this again after the waitForCondition call?
> 
> Also, shall we increase the scope of the expected string variable and reuse it (the same string is used again a few times)?

It was done originally so I kept it, I guess it's a style question.

> Also, shall we increase the scope of the expected string variable and reuse it (the same string is used again a few times)?

I think that's difficult to do because most of the assertions are inside ContentTasks.

> I'm not sure I fully understand the exact order of events, but how come we don't need a waitForCondition here like above?

Uhm, good question. I just transformed the original test, not sure what they were thinking. I think either way works so I'd prefer to keep it like that and not risk breaking anything.

Comment 14

[Johann Hofmann [:johannh]]

I had forgotten to do a try push for this, doing it now.

Comment 15

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 442f9e522c80 -d 0a543fedee9c: rebasing 414214:442f9e522c80 "Bug 1368143 - Modernize browser_bug902156.js. r=nhnt11" (tip)
merging browser/base/content/test/siteIdentity/browser_bug902156.js
warning: conflicts while merging browser/base/content/test/siteIdentity/browser_bug902156.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 16

[Johann Hofmann [:johannh]]

Comment on attachment 8894495 [details]
Bug 1368143 - Modernize browser_bug902156.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/165664/diff/2-3/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b850c8c06d17
Modernize browser_bug902156.js. r=nhnt11

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 19

[Intermittent Failures Robot]

6 failures in 949 pushes (0.006 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 4
* mozilla-inbound: 2

Platform breakdown:
* osx-10-10: 6

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1368143&startday=2017-08-14&endday=2017-08-20&tree=all