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)
Core Graveyard
Plug-ins
Tracking
(firefox24 fixed, firefox25 fixed, firefox26 fixed)
RESOLVED
FIXED
mozilla26
People
(Reporter: emorley, Assigned: gfritzsche)
References
Details
(Keywords: intermittent-failure)
Attachments
(1 file, 1 obsolete file)
32.53 KB,
patch
|
keeler
:
review+
bajaj
:
approval-mozilla-aurora+
bajaj
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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 }
Assignee | ||
Comment 1•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Priority: -- → P3
Assignee | ||
Comment 2•11 years ago
|
||
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)
Reporter | ||
Updated•11 years ago
|
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)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 8•11 years ago
|
||
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)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 10•11 years ago
|
||
(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?
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 20•11 years ago
|
||
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 */ });
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) |
Assignee | ||
Comment 31•11 years ago
|
||
Note: comment 22 and comment 24 were factored out to bug 897935.
Assignee | ||
Comment 33•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
OS: Mac OS X → All
Hardware: x86_64 → All
Assignee | ||
Comment 34•11 years ago
|
||
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
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 36•11 years ago
|
||
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 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 53•11 years ago
|
||
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 hidden (Legacy TBPL/Treeherder Robot) |
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-
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 57•11 years ago
|
||
(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.
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) |
Assignee | ||
Comment 66•11 years ago
|
||
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)
Assignee | ||
Updated•11 years ago
|
Attachment #785729 -
Attachment description: Delay running plugin notification tests until after PluginBindingAttached → Delay running plugin notification tests until after PluginBindingAttached, v2
Assignee | ||
Comment 67•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=1d7d16e16a8d
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+
Assignee | ||
Comment 69•11 years ago
|
||
Thanks for the quick turn-around. https://hg.mozilla.org/integration/mozilla-inbound/rev/ffcf35089109
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 72•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ffcf35089109
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 74•11 years ago
|
||
Please request Aurora uplift on this. :)
Comment 75•11 years ago
|
||
And beta.
Assignee | ||
Comment 76•11 years ago
|
||
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?
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Updated•11 years ago
|
Attachment #785729 -
Flags: approval-mozilla-beta?
Attachment #785729 -
Flags: approval-mozilla-beta+
Attachment #785729 -
Flags: approval-mozilla-aurora?
Attachment #785729 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 79•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/13a012060eb9 https://hg.mozilla.org/releases/mozilla-beta/rev/6f6f59093d46
Comment hidden (Legacy TBPL/Treeherder Robot) |
Assignee | ||
Comment 81•11 years ago
|
||
(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.
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) |
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•