Closed Bug 896965 Opened 11 years ago Closed 11 years ago

Intermittent browser_pluginnotification.js, browser_CTP_data_urls.js | Test N, Should {now} have a click-to-play notification (or Should have displayed the missing plugin notification)

Categories

(Core Graveyard :: Plug-ins, defect, P3)

defect

Tracking

(firefox24 fixed, firefox25 fixed, firefox26 fixed)

RESOLVED FIXED
mozilla26
Tracking Status
firefox24 --- fixed
firefox25 --- fixed
firefox26 --- fixed

People

(Reporter: emorley, Assigned: gfritzsche)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 1 obsolete file)

Rev4 MacOSX Snow Leopard 10.6 mozilla-central opt test mochitest-browser-chrome on 2013-07-22 14:13:48 PDT for push bda9723bdccc

slave: talos-r4-snow-065

https://tbpl.mozilla.org/php/getParsedLog.php?id=25577066&tree=Mozilla-Central

{
14:18:12     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 7, application/x-test should not be a missing plugin
14:18:12     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 8, Should not have displayed the missing plugin notification
14:18:12     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 8, Should not be a missing plugin list
14:18:12  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 8, Should have a click-to-play notification
14:18:12     INFO -  Stack trace:
14:18:12     INFO -      JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js :: test8 :: line 210
14:18:12     INFO -      JS frame :: chrome://mochikit/content/browser-test.js :: testScope/test_executeSoon/<.run :: line 589
14:18:12     INFO -      native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
14:18:12     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 8, Found plugin in page
14:18:12     INFO -  TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 8, plugin fallback type should be PLUGIN_CLICK_TO_PLAY
}
This is similar to bug 896676 and bug 896438.
Something results in the click-to-play notification not, or not yet, being present after page load and the executeSoon() delay.

In all 3 bugs exhibiting this behavior nothing like JS errors is showing up.
Something with the new doorhanger code or one of its follow-ups must have
* changed the timing of the events/notifications or
* broke something related to activating that notification
Priority: -- → P3
Blocks: 880735
Looking through the changes for bug 889788 i wonder if we could end up scheduling the "PluginRemoved" event [1] after the events triggering showing the notification were already scheduled - hence removing the notification later via [2].

Benjamin, does this scenario make any sense to you?

[1] http://hg.mozilla.org/mozilla-central/annotate/fb4bf993a58a/content/base/src/nsObjectLoadingContent.cpp#l687
[2] https://hg.mozilla.org/mozilla-central/annotate/fb4bf993a58a/browser/base/content/browser-plugins.js#l787
Flags: needinfo?(benjamin)
Summary: Intermittent browser_pluginnotification.js | Test 8, Should have a click-to-play notification → Intermittent browser_pluginnotification.js | Test N, Should {now} have a click-to-play notification (or Should have displayed the missing plugin notification)
Well, in none of the tests should we be completely removing the notification (since there are still plugins on the page). However, it is possible that we would be recreating the notification, because we currently do that whenever we hit any of those events.

I have a plan to show the notification only once, but that involves some work with icons and PopupNotifications.jsm, since I don't think we can change the icon dynamically currently.

The other possibility is that this is caused by the delay at http://hg.mozilla.org/mozilla-central/rev/3f3a0ed44893#l15.14

If that's the case, then an extra .executeSoon may be necessary, or we may need to change the wait conditions of some tests.
Flags: needinfo?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #8)
> Well, in none of the tests should we be completely removing the notification
> (since there are still plugins on the page).

Ok, i'm not familiar with how this is supposed to work - i see that in the case of "PluginRemoved" we grab the document from the event.
When navigating, is the document still the same?
The binding is only attached when layout flushes, right? Ergo, with the setTimeout in firing the event, this should do:

> [ ... Create plugin and bind to tree ... ]
> plugin.clientTop; // Event is now in queue
> SimpleTest.executeSoon(() => { /* Will execute after event fires */ });
Note: comment 22 and comment 24 were factored out to bug 897935.
https://tbpl.mozilla.org/php/getParsedLog.php?id=25811083&tree=Fx-Team
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_CTP_data_urls.js | Test 2a, Should have a click-to-play notification
Summary: Intermittent browser_pluginnotification.js | Test N, Should {now} have a click-to-play notification (or Should have displayed the missing plugin notification) → Intermittent browser_pluginnotification.js, browser_CTP_data_urls.js | Test N, Should {now} have a click-to-play notification (or Should have displayed the missing plugin notification)
OS: Mac OS X → All
Hardware: x86_64 → All
I caught instances of those failures on a try run with some debug dumping [1] and it's pretty clear that "PluginBindingAttached" comes after the test function is run:
https://tbpl.mozilla.org/php/getParsedLog.php?id=25834160&tree=Try&full=1#error0
https://tbpl.mozilla.org/php/getParsedLog.php?id=25834138&tree=Try&full=1#error0
https://tbpl.mozilla.org/php/getParsedLog.php?id=25834161&tree=Try&full=1#error0

So it looks like we need to do what johns suggested in comment 20.

[1] https://tbpl.mozilla.org/?tree=Try&rev=e041fe71b2e2
Assignee: nobody → georg.fritzsche
This looks good on try [1]. Jaws, can you review this?

This does what johns suggested - for tests depending on plugin notifications flush layout, thus triggering "PluginBindingAttached" and schedule the test to run afterwards.

[1] https://tbpl.mozilla.org/?tree=Try&rev=e3fa31308a3a
Attachment #782539 - Flags: review?(jaws)
Comment on attachment 782539 [details] [diff] [review]
Delay running plugin notification tests until after PluginBindingAttached

Redirecting review since I don't think I'll have time to get to this.
Attachment #782539 - Flags: review?(jaws) → review?(dkeeler)
Comment on attachment 782539 [details] [diff] [review]
Delay running plugin notification tests until after PluginBindingAttached

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

Looks good. However, I have a couple of questions/suggestions, so r- for now.
One thing is I'm concerned about the miscellaneous browser_bug??????.js tests that involve plugins. It's possible those need similar modifications.

::: browser/base/content/test/browser_CTP_data_urls.js
@@ +94,5 @@
>    gNextTest = nextTest;
>    gTestBrowser.contentWindow.location = url;
>  }
>  
> +function runAfterPluginBindingAttached(func, doc) {

can this be put in some sort of head.js?

::: browser/base/content/test/browser_pluginnotification.js
@@ +93,5 @@
>    gNextTest = nextTest;
>    gTestBrowser.contentWindow.location = url;
>  }
>  
> +function runAfterPluginBindingAttached(func, doc) {

It doesn't look like doc is ever specified, so this parameter is probably not necessary.
Also, maybe add a comment to the effect of "this returns a function that forces a plugin binding to attach and then queues the given function to run after" or something.

@@ +102,5 @@
> +    let elems = doc.getElementsByTagName('embed');
> +    if (elems.length < 1) {
> +      elems = doc.getElementsByTagName('object');
> +    }
> +    elems[0].clientTop;

are we always guaranteed to have at least one embed or object? I think there are some tests that start out with no plugins present... (or, if there aren't, then maybe there should be)

@@ +103,5 @@
> +    if (elems.length < 1) {
> +      elems = doc.getElementsByTagName('object');
> +    }
> +    elems[0].clientTop;
> +    SimpleTest.executeSoon(func);

I imagine we can call executeSoon directly like in other parts of this test

@@ +122,5 @@
>  
>    var plugin = getTestPlugin();
>    ok(plugin, "Should have a test plugin");
>    plugin.enabledState = Ci.nsIPluginTag.STATE_ENABLED;
> +  prepareTest(runAfterPluginBindingAttached(test2), gTestRoot + "plugin_test.html");

If we're changing every call to prepareTest to wrap a function with runAfterPluginBindingAttached, we could just modify prepareTest to do the wrapping for us (otherwise, nevermind) (this applies to both test files modified in this patch).
Attachment #782539 - Flags: review?(dkeeler) → review-
(In reply to David Keeler (:keeler) from comment #55)
> One thing is I'm concerned about the miscellaneous browser_bug??????.js
> tests that involve plugins. It's possible those need similar modifications.

Good point.

> ::: browser/base/content/test/browser_CTP_data_urls.js
> @@ +94,5 @@
> >    gNextTest = nextTest;
> >    gTestBrowser.contentWindow.location = url;
> >  }
> >  
> > +function runAfterPluginBindingAttached(func, doc) {
> 
> can this be put in some sort of head.js?

I was thinking about that, but AFAICT this function couldn't depend on gTestBrowser and would always have to receive the doc or browser explicitly. See also e.g. prepareTest() being duplicated.

Any better suggestions or do you think i can go with this?

> @@ +102,5 @@
> > +    let elems = doc.getElementsByTagName('embed');
> > +    if (elems.length < 1) {
> > +      elems = doc.getElementsByTagName('object');
> > +    }
> > +    elems[0].clientTop;
> 
> are we always guaranteed to have at least one embed or object? I think there
> are some tests that start out with no plugins present... (or, if there
> aren't, then maybe there should be)

This is for forcing "PluginBindingAttached", which only is useful when there are plugins in the document.

> @@ +122,5 @@
> >  
> >    var plugin = getTestPlugin();
> >    ok(plugin, "Should have a test plugin");
> >    plugin.enabledState = Ci.nsIPluginTag.STATE_ENABLED;
> > +  prepareTest(runAfterPluginBindingAttached(test2), gTestRoot + "plugin_test.html");
> 
> If we're changing every call to prepareTest to wrap a function with
> runAfterPluginBindingAttached, we could just modify prepareTest to do the
> wrapping for us (otherwise, nevermind) (this applies to both test files
> modified in this patch).

Not all and this way it's also usable in the (rare) cases where we do custom navigation etc. and set gNextTest directly instead of through prepareTest().
(In reply to Georg Fritzsche [:gfritzsche] from comment #57)
> (In reply to David Keeler (:keeler) from comment #55)
> > ::: browser/base/content/test/browser_CTP_data_urls.js
> > @@ +94,5 @@
> > >    gNextTest = nextTest;
> > >    gTestBrowser.contentWindow.location = url;
> > >  }
> > >  
> > > +function runAfterPluginBindingAttached(func, doc) {
> > 
> > can this be put in some sort of head.js?
> 
> I was thinking about that, but AFAICT this function couldn't depend on
> gTestBrowser and would always have to receive the doc or browser explicitly.
> See also e.g. prepareTest() being duplicated.

Oh, right - good call.

> Any better suggestions or do you think i can go with this?

No, I think this is fine - it's only a test, anyway. If we build on this and/or it gets too cumbersome, we can refactor in a follow-up.

> > @@ +102,5 @@
> > > +    let elems = doc.getElementsByTagName('embed');
> > > +    if (elems.length < 1) {
> > > +      elems = doc.getElementsByTagName('object');
> > > +    }
> > > +    elems[0].clientTop;
> > 
> > are we always guaranteed to have at least one embed or object? I think there
> > are some tests that start out with no plugins present... (or, if there
> > aren't, then maybe there should be)
> 
> This is for forcing "PluginBindingAttached", which only is useful when there
> are plugins in the document.

Sounds good.

> > @@ +122,5 @@
> > >  
> > >    var plugin = getTestPlugin();
> > >    ok(plugin, "Should have a test plugin");
> > >    plugin.enabledState = Ci.nsIPluginTag.STATE_ENABLED;
> > > +  prepareTest(runAfterPluginBindingAttached(test2), gTestRoot + "plugin_test.html");
> > 
> > If we're changing every call to prepareTest to wrap a function with
> > runAfterPluginBindingAttached, we could just modify prepareTest to do the
> > wrapping for us (otherwise, nevermind) (this applies to both test files
> > modified in this patch).
> 
> Not all and this way it's also usable in the (rare) cases where we do custom
> navigation etc. and set gNextTest directly instead of through prepareTest().

Sounds good.
This addresses the above comments and fixes all the tests browser/base/content/test/ that didn't have some way of handling this problem already.
Note that i did not add the function in bug-specific tests that only needed a fix for one test-function.
Attachment #782539 - Attachment is obsolete: true
Attachment #785729 - Flags: review?(dkeeler)
Attachment #785729 - Attachment description: Delay running plugin notification tests until after PluginBindingAttached → Delay running plugin notification tests until after PluginBindingAttached, v2
Comment on attachment 785729 [details] [diff] [review]
Delay running plugin notification tests until after PluginBindingAttached, v2

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

Awesome - thanks for doing this, Georg.
Attachment #785729 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/ffcf35089109
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Please request Aurora uplift on this. :)
And beta.
Comment on attachment 785729 [details] [diff] [review]
Delay running plugin notification tests until after PluginBindingAttached, v2

To my surprise this actually applies fine as-is on Aurora and Beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 880735 et al
User impact if declined: Increased orange count and sad sheriffs.
Testing completed (on m-c, etc.): Fine on m-c, oranges gone.
Risk to taking this patch (and alternatives if risky): None, test-only.
String or IDL/UUID changes made by this patch: None.
Attachment #785729 - Flags: approval-mozilla-beta?
Attachment #785729 - Flags: approval-mozilla-aurora?
Attachment #785729 - Flags: approval-mozilla-beta?
Attachment #785729 - Flags: approval-mozilla-beta+
Attachment #785729 - Flags: approval-mozilla-aurora?
Attachment #785729 - Flags: approval-mozilla-aurora+
Depends on: 902530
Depends on: 906007
(In reply to TinderboxPushlog Robot from comment #80)
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/browser/browser/base/content/test/
> browser_pluginnotification.js | Test 1, Should have displayed the missing
> plugin notification

So i missed that the "plugins-not-found" notification also depends on the binding and hence test1() should be wrapped the same way.
Depends on: 906703
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: