Closed Bug 1160788 Opened 9 years ago Closed 9 years ago

Intermittent e10s browser_CTP_hide_overlay.js | overlay should be hidden

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(e10s+, firefox40 disabled, firefox41 disabled, firefox42 fixed, firefox-esr31 unaffected, firefox-esr38 unaffected)

RESOLVED FIXED
mozilla42
Tracking Status
e10s + ---
firefox40 --- disabled
firefox41 --- disabled
firefox42 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected

People

(Reporter: philor, Assigned: rowbot)

References

(Blocks 1 open bug)

Details

(Keywords: intermittent-failure)

Attachments

(2 files, 2 obsolete files)

      No description provided.
Blocks: e10s-tests
tracking-e10s: --- → +
Timing very-closely follows bug 1129040.
Flags: needinfo?(jmathies)
Jim, can we please just disable this test if there isn't time to investigate and fix it?
Test disabled.
https://hg.mozilla.org/integration/fx-team/rev/1debdc69d5e9
Flags: needinfo?(jmathies)
Whiteboard: [test disabled][leave open]
Flags: needinfo?(ryanvm)
I felt the need to take a stab at this.  I have no concrete evidence that this will actually fix the problem, just a hunch.

This gets rid of the second ContentTask and fixes a spelling mistake in the comment.

Jim, since you had a previous ni? in this bug, I pick you to review!
Attachment #8633906 - Flags: review?(jmathies)
Comment on attachment 8633906 [details] [diff] [review]
bug1160788_do_not_spawn_multiple_content_tasks.diff

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

Have you tried pushing to try for some retriggers? I'm skeptical, but lets see. Honestly I'd expect this change to cause more failures. Lets make sure it fixes something before landing.
Attachment #8633906 - Flags: review?(jmathies) → review+
Another option would be to move that second check block into its own task. That should give content some time to settle before this check is made.

One possible cause of this might be bug 1160166.
Talked to :jimm about this on IRC a little bit.  He is going to push this to try for me to see what happens.  Just to summarize:

:jimm is right to be skeptical about this change, but the reason I made this change is because I wrote the second test in this file, which was pretty much a straight copy/paste of the failing test here, and that one, as far as I can tell isn't failing.  The 2 primary differences between the 2 tests are that the failing one uses 2 ContentTasks instead of 1 and it does some additional setup steps on lines 18-22.  So, I figured by process of elimination, we could figure this out.  Also, :jimm confirmed that the comment for the test is wrong, so bug 1160166 shouldn't be causing any issues here.  I don't think the content should need time to settle, if it does, that sounds like a really serious problem if calling classList.contains right after a classList.remove gives the wrong result.
Flags: needinfo?(jmathies)
Didn't mean to set ni?
Flags: needinfo?(jmathies)
Well, that try came back all green.  Not entirely sure if that just means I was lucky and the intermittent didn't occur or if this actually fixed it.

If this is the later case, then its begs the question of why did it fix it.  Since the problem occurred when using 2 ContentTasks, but works with 1, I am speculating that this is not a timing issue, which pretty much just leaves ContentTask itself as the potential culprit here.
Assignee: nobody → smokey101stair
Flags: needinfo?(jmathies)
I don't see it being re-enabled in the Try push? Or was it on top of a parent rev before the test was disabled?
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #442)
> I don't see it being re-enabled in the Try push? Or was it on top of a
> parent rev before the test was disabled?

I did a quick search of the raw logs for the try run, and it appears that the test did run or at least I assume that is what the following means:

15:45:24     INFO -  24 INFO TEST-START | browser/base/content/test/plugins/browser_CTP_hide_overlay.js
15:45:24     INFO -  MEMORY STAT | vsize 3491MB | residentFast 441MB | heapAllocated 127MB
15:45:24     INFO -  25 INFO TEST-OK | browser/base/content/test/plugins/browser_CTP_hide_overlay.js | took 328ms
In that case, a single isolated result for an intermittent failure isn't by itself very informative. You'll need to retrigger the run many times (probably 10-20) to have a degree of confidence that it actually helped.
Ah, ok.  Thanks for the explanation.  I'm new to the try server so I wasn't sure if a test could be triggered multiple times in one run or if multiple runs were needed. Well, then.  More try runs!  Now I feel bad for having to bug someone to trigger this over and over again since I don't have permission to push to try myself, haha.
You can absolutely retrigger existing runs. That's the strongly-preferred method.
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Oooh! Thanks for the info.
I guess I don't have permission to retrigger existing runs either. :(  Would you mind retriggering this for me?
Yeah, you need Level 1 commit access to do so. I triggered 20 runs on the relevant chunks :)
Thanks!
The original failures where e10s test failures but I didn't get e10s test runs in that try push. Sorry, have to push to try again.
Flags: needinfo?(jmathies)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #424)
> Test disabled.
> https://hg.mozilla.org/integration/fx-team/rev/1debdc69d5e9

The bad failure here was on osx. We shouldn't have disabled on other platforms.
(In reply to Jim Mathies [:jimm] from comment #452)
> The bad failure here was on osx. We shouldn't have disabled on other
> platforms.

A cursory scan of this bug showed failures across all 3 platforms and the needinfo request had gone unanswered for nearly a month.
(In reply to Jim Mathies [:jimm] from comment #453)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce69e64a4bcd

Seems fixed with this patch, can we re-enable?
Flags: needinfo?(ryanvm)
Worth a shot.
Flags: needinfo?(ryanvm)
I'm still curious as to why having multiple ContentTasks would cause this to fail.  Other than the fact that having 2 ContentTasks was unnecessary, there was nothing wrong with the code that I could tell, and it should have worked just fine.
Since this patch seems to do the trick, I fixed the comment for the test so that it no longer incorrectly states that we are testing a disabled plugin.
Attachment #8633906 - Attachment is obsolete: true
Attachment #8634780 - Flags: review?(jmathies)
Comment on attachment 8634780 [details] [diff] [review]
bug1160788_do_not_spawn_multiple_content_tasks_v2.diff

In your commit message please be more specific, something like - 

Bug 1160788 - Attempt to fix random orange: don't spawn multiple ContentTasks when checking the visible state of the click-to-play overlay. r=jimm"

Thanks for working on this! If you're interested in working on another orange very similar to this, take a look at bug 1160780.
Attachment #8634780 - Flags: review?(jmathies) → review+
> Thanks for working on this! If you're interested in working on another orange very similar to this, take a look at bug 1160780.
Trying to pawn off all the oranges to me, eh?! haha, yeah, I'll take a look.  Looking can't hurt, right?  Well, if I feel a sharp pain in my eye, I'll know who to talk to. :)
Keywords: checkin-needed
Whiteboard: [test disabled][leave open]
https://hg.mozilla.org/mozilla-central/rev/bcc870c7d75f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Ryan, it doesn't look like this test ever got re-enabled.  Do you want me to make a patch to do that or is it easier if you back out your patch?
Flags: needinfo?(ryanvm)
Talked with :jimm.  I'll post a patch to re-enable the test.
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm)
Resolution: FIXED → ---
This re-enables the browser_CTP_hide_overlay test.
Attachment #8636809 - Flags: review?(jmathies)
Attachment #8636809 - Flags: review?(jmathies) → review+
Keywords: checkin-needed
Attachment #8634863 - Attachment description: bug1160788_do_not_spawn_multiple_content_tasks_v3.diff → (checked in) bug1160788_do_not_spawn_multiple_content_tasks_v3.diff
https://hg.mozilla.org/mozilla-central/rev/c252daa6a8e7
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
ni? myself to push the test fix and re-enabling to Aurora next time I'm pushing there since this is looking stable.
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.