Closed Bug 625651 Opened 14 years ago Closed 7 years ago

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

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED INCOMPLETE

People

(Reporter: philor, Unassigned)

References

Details

(Keywords: intermittent-failure, regression, Whiteboard: [test disabled on Windows; see comment 46])

Attachments

(2 files)

Disable on Windows for now
811 bytes, patch
roc
: review+
Details | Diff | Splinter Review
Another version to fix this bug
1.31 KB, patch
Details | Diff | Splinter Review 
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.
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)
Attachment #503741 - Attachment mime type: application/octet-stream → text/plain
I think only bsmeberg can test this on windows.
Attachment #503741 - Attachment is patch: true
don't understand why this test does not fail on linux, and fails only on windows
Keywords: checkin-needed
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]
Whiteboard: [orange] → [orange][test disabled on Windows; see comment 46]
There you go, there's a Linux failure.
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?
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
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?
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).
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.
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
Closed: 7 years ago
Resolution: --- → INCOMPLETE
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: