Very frequent failure in test_xulbrowser_plugin_visibility.xul | Plugin in tab 1 never became visible. after landing of bug 619176

RESOLVED INCOMPLETE

Status

()

defect
--
major
RESOLVED INCOMPLETE
8 years ago
2 years ago

People

(Reporter: philor, Unassigned)

Tracking

({intermittent-failure, regression})

Trunk
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [test disabled on Windows; see comment 46])

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
I screwed up - this should have resulted in a quick fix or a backout first thing this morning, but the summary was close enough to that of bug 612448 that I didn't notice for 12 hours that I was starring a new and very frequent failure as though it was an old timeout which actually won't occur anymore, because it only happened on slaves we're no longer using.

The first failure,

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1294933601.1294935205.3329.gz
Rev3 WINNT 6.1 mozilla-central opt test mochitest-other on 2011/01/13 07:46:41
s: talos-r3-w7-005

7555 INFO TEST-START | chrome://mochitests/content/chrome/modules/plugin/test/test_xulbrowser_plugin_visibility.xul
7556 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/modules/plugin/test/test_xulbrowser_plugin_visibility.xul | Plugin in tab 1 never became visible.
7557 INFO TEST-END | chrome://mochitests/content/chrome/modules/plugin/test/test_xulbrowser_plugin_visibility.xul | finished in 5212ms

was in the build which included "http://hg.mozilla.org/mozilla-central/rev/b9029c71a63a - Oleg Romashin - Bug 619176 - Plugins get Visible state every time when scrolling (:BuildLayer always make them visible). r=roc a=approval2.0" which seems like a rather smokey gun.
(Reporter)

Comment 1

8 years ago
I'm not sure what we want to do here, but I am sure we don't want to leave it just failing, since it's going to be around triple the failure rate of our next worst intermittent.
Attachment #503741 - Flags: review?(roc)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Attachment #503741 - Attachment mime type: application/octet-stream → text/plain
I think only bsmeberg can test this on windows.
(Reporter)

Updated

8 years ago
Attachment #503741 - Attachment is patch: true
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
don't understand why this test does not fail on linux, and fails only on windows
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
(Reporter)

Updated

8 years ago
Keywords: checkin-needed
(Reporter)

Comment 10

8 years ago
Forgot the comment that goes with that check-needed, that'll be invisible in a sea of tbplbot comments: please leave the bug open after pushing that temporary disable patch.
Whiteboard: [orange] → [orange][leave open]
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Whiteboard: [orange] → [orange][test disabled on Windows; see comment 46]
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
(Reporter)

Comment 52

8 years ago
There you go, there's a Linux failure.
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
This is a new failure caused by a patch which I had reservations about to begin with. I really think we ought to back out bug 619176 here. roc, do you disagree?
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Can we somehow land suggested patch?
Comment on attachment 503741 [details] [diff] [review]
Disable on Windows for now

No. AFAICT the test is correct, and your patch is wrong. Your patch should be backed out, I believe.
I think something else should be fixed here, we should not disable that for test only, and make it works for test and not test environment..

with this visibility call we make plugins which are not visible on the screen visible again, and that is wrong..
Probably we should make some sort of test for that issue
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Ted asked me to revert b9029c71a63a, also we need to revert patch from this bug also...

Test disable patch seems lost it's context and probably philor is better person to revert that.
Also I tried to push backout for my patch but tree looks not very green and I was not able to figure out how to star all failures.

Phil could you backout test disable patch any my patch in one commit when tree is green?
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Attachment #505807 - Flags: review?(roc)
Comment on attachment 505807 [details] [diff] [review]
Another version to fix this bug

What is the bug here and how does this patch fix it?
Related bug 612448.
Problem here that we do set force Visibility = true only when reftests are running.

if we enable this as TRUE without any condition, then it might fix some tests, but will make plugins visible even if they are not in visible viewport.

This check will set plugins visibility as true only when they are really visible  in scroll rect (not CSS visibility).
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Oleg, I think your patch is going to mark a partially-visible plugin as not visible. That would be bad.

But I still don't understand why the plugin never becomes visible with our current code. Is kTimeout too low? Probably we should just get rid of paintGiveUp and let the standard mochitest timeout take over.
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Comment hidden (Legacy TBPL/Treeherder Robot)
Whiteboard: [orange][test disabled on Windows; see comment 46] → [test disabled on Windows; see comment 46]
Resolving old bugs which are likely not relevant any more, since NPAPI plugins are deprecated.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.