Closed Bug 759703 Opened 12 years ago Closed 12 years ago

Intermittent browser_pluginnotification.js | Test 16b, Should have a click-to-play notification | Test 16c, Should not have a click-to-play notification | Test 16c, Plugin should be activated

Categories

(Core Graveyard :: Plug-ins, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla16

People

(Reporter: emorley, Assigned: keeler)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

Rev3 Fedora 12x64 mozilla-inbound debug test mochitest-other on 2012-05-30 02:45:07 PDT for push 8f8639307984

slave: talos-r3-fed64-016

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

{
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16a, Should not have a click-to-play notification
--DOMWINDOW == 42 (0x77acb50) [serial = 1036] [outer = 0x7761f50] [url = chrome://mochitests/content/browser/browser/base/content/test/plugin_clickToPlayAllow.html]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16b, Should have a click-to-play notification
Stack trace:
    JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js :: test16b :: line 477
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16b, Plugin should not be activated
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16c, Should not have a click-to-play notification
Stack trace:
    JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js :: test16c :: line 488
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16c, Plugin should be activated
Stack trace:
    JS frame :: chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js :: test16c :: line 491
    native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0

TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16d, Should have a click-to-play notification
TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | Test 16d, Plugin should not be activated
WARNING: NS_ENSURE_TRUE(mMutable) failed: file ../../../../netwerk/base/src/nsSimpleURI.cpp, line 258
INFO TEST-END | chrome://mochitests/content/browser/browser/base/content/test/browser_pluginnotification.js | finished in 7056ms
}
Attached patch fix idea (obsolete) — Splinter Review
Ideally, we'd want some kind of event when the plugin is actually in the page and ready to be clicked on. The only event I could find that actually fired was "focus" ("load" doesn't seem to fire for objects or embeds (?!)). So, I set up an interval that would send tab key events. When the plugin shows up in the page, it will eventually be focused by the tabbing, and the listener will go on to the next test. If the plugin doesn't show up after 5 seconds' worth of tabbing, the test moves on.

Also, I thought it would be a good idea to get rid of all other timeouts, because the same thing could happen with them. Most of the time we're waiting for a plugin to be activated, so I added other intervals that basically do the same thing.
Attachment #630199 - Flags: feedback?(jaws)
Comment on attachment 630199 [details] [diff] [review]
fix idea

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

Interesting solution. Maybe this will work, but the failures are very infrequent so I'm not sure how useful this change is. What do you think?

::: browser/base/content/test/browser_pluginnotification.js
@@ +476,5 @@
>  
>    prepareTest(test16a, gTestRoot + "plugin_bug743421.html");
>  }
>  
> +function sendTab(aWindow) {

please rename this to sendTabKey to disambiguate with browser tabs.

@@ +532,5 @@
> +  var interval = setInterval(function() { 
> +    if (tries < 50) {
> +      sendTab(gTestBrowser.contentWindow); 
> +    } else {
> +      callback();

We shouldn't be hitting these |else| blocks here nor in the other test functions within this file. Can you fail a test if it reaches one of these |else| blocks or if |tries| goes over its limit?
Attachment #630199 - Flags: feedback?(jaws)
Thanks for the feedback. I'll make the changes when I get a chance.
I think it's worth it to fix for two reasons: 1) it makes the test faster, because it only waits as long as it has to (within 100ms or whatever we want to set that number to) and 2) fewer intermittent oranges means a better test suite means better development, so it's good for the project.
Attachment #630199 - Flags: feedback+ → review+
Attached patch patch (obsolete) — Splinter Review
I refactored things, and I think this is a much better solution. We no longer need that tab-sending business. I wasn't quite sure what you meant about the else blocks, but I made sure the test failed if it waited too long (tries hit the limit). I also sped up the interval, so the test goes even faster.
Attachment #631027 - Flags: review?(jaws)
Attachment #630199 - Attachment is obsolete: true
Comment on attachment 631027 [details] [diff] [review]
patch

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

::: browser/base/content/test/browser_pluginnotification.js
@@ +252,5 @@
>    ok(!objLoadingContent.activated, "Test 9a, Plugin with id=" + plugin2.id + " should not be activated");
>  
>    EventUtils.synthesizeMouse(plugin1, 100, 100, { });
> +  var objLoadingContent = plugin1.QueryInterface(Ci.nsIObjectLoadingContent);
> +  var condition = function() { return objLoadingContent.activated; };

This could use an expression closure here if you wanted. E.g. |var condition = function() objLoadingContent.activated;| See https://developer.mozilla.org/en/New_in_JavaScript_1.8#Expression_closures_%28Merge_into_own_page.2Fsection%29 for more information. No added benefit, but I just thought I could mention this since it's cool and is less typing :)
Attachment #631027 - Flags: review?(jaws) → review+
Attached patch patchSplinter Review
Decided to use expression closures - carrying over r+
Try run: https://tbpl.mozilla.org/?tree=Try&rev=115fa4447f2b
Assignee: nobody → dkeeler
Attachment #631027 - Attachment is obsolete: true
Attachment #631138 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/700a03cd53d4
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/700a03cd53d4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [orange]
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: