Bug 899347 - Make click-to-play work in e10s

GWAERRRR - there have been changes in browser-plugins since mbrubeck worked on this stuff. The patch doesn't apply cleanly, and… well, I don't want to just clobber everything.

So - I need to figure out what's changed since mbrubeck worked on this stuff, and then see what in mbrubeck's patch I need to modify in order for it all to keep working.

mbrubeck posted his patch around July 1st. Commits to browser-plugins around that time:

Birunthan Mohanathas - Bug 994708 - Part 2: Record submission event for plugin crashes. r=gps

This is pretty straight forward - it's just decorating the crash submission with the plugin process type.

Felipe Gomes - Bug 1009760 - Support displaying of crash notification for GMP plugins. r=gfritzsche

This one… is more complicated. GMP = GeckoMediaPlugins . These are 3rd party codecs, EME and CDMs.

So we've added a new type of plugin… and felipe added support for showing when they crash. Simple enough.

Let's take a closer look at the patch:

https://hg.mozilla.org/mozilla-central/rev/41df67208b1b

This reorganized a number of things in browser-plugins.js. Oooh! and it added a test. Good.

Georg Fritzsche - Bug 1043531 - Let frontend deal properly with PluginCrashed without a browser dump id. r=ttaubert

This seems pretty straight forward… not worried about this one.

Georg Fritzsche - Bug 1045500 - Skip processing the plugin name for plugin crashes in the front-end for GMP plugins. r=ttaubert

Also pretty straight forward…

Birunthan Mohanathas - Bug 972237 - Fix missing plugin overlay with some zoom levels and add test. r=mikedeboer

This seems also straight forward - and changes how the plugin overlay works with zoom levels. And its got a test!!

So it's felipe's patch in bug 1009760 that I need to integrate. Now let's look and see what mbrubeck's patch did.

Adds an initialize function that adds message listeners to all sorts of plugin messages.

Adds an "_unremotePrincipal" function that will take some CPOW'd principal or object with principal properties, and turn it into a real nsIPrincipal in the chrome process.

The event handler is removed, and I'm guessing it gets moved into content.js. (Well, kinda - it gets moved to PluginContent.jsm, which is imported into content.js).

Handlers for click-to-play get removed, and again, I'm guessing it gets moved to content.js. (Same as above).

Ahhh, interesting - so mbrubeck copied browser/base/content/browser-plugins to browser/modules/PluginContent.jsm, and then did a ton of mods on it. I guess that helps preserve history instead of a manual copy paste.

I wonder if maybe this would be a lot simpler if I updated my tree back to when mbrubeck was working on this, applied the patch, and then attempted to rebase it on tip?

Let's try that.

Hm… seems to have kinda worked. handleEvent seems to have diverged quite a bit… I wonder if mbrubeck did some cleanup in here…

I… I kinda have stuff working kinda. For non-e10s, anyway. The crashinfobar test shows an info bar. That's good.

The notification bar isn't being detected in the test. Why? Because the frame script hears the event, fires an async message, but after it fires it, the event keeps bubbling up and the test expects the notification to appear once it sees the event. When the event is done bubbling, that's when the message is handled and the notification bar is displayed.

So we've changed the order of events. Again. So I need a way of waiting for the message to be handled before checking for the existence of the notification.

Is there prior art for waiting for a notification? It's unfortunate that it doesn't seem to emit events on showing… hm - I see several instances of us using waitForCondition for this sort of thing. I used it, and that seemed to fix that test.

I'm looking at browser/base/content/test/plugins/browser_CTP_crashreporting.js now, and it looks like this one is busted because it attempts to read information about the content - like, it tests style attributes, checkbox values, etc.

browser_CTP_crashreporting.js and browser_pluginCrashCommentAndURL.js

I'm actually finding only two instances of this occurring - one in browser_CTP_crashreporting.js and browser_pluginCrashCommentAndURL.js. Here are the two usages:

function onCrash() {
try {
let plugin = gBrowser.contentDocument.getElementById("plugin");
let elt = gPluginHandler.getPluginUI.bind(gPluginHandler, plugin);
let style =
gBrowser.contentWindow.getComputedStyle(elt("pleaseSubmit"));
is(style.display,
currentRun.shouldSubmissionUIBeVisible ? "block" : "none",
"Submission UI visibility should be correct");
if (!currentRun.shouldSubmissionUIBeVisible) {
// Done with this run.
doNextRun();
return;
}
elt("submitComment").value = currentRun.comment;
elt("submitURLOptIn").checked = currentRun.urlOptIn;
elt("submitButton").click();
// And now wait for the submission status notification.
}
catch (err) {
failWithException(err);
doNextRun();
}
}

function onCrash() {
try {
let plugin = gBrowser.contentDocument.getElementById("test");
let elt = gPluginHandler.getPluginUI.bind(gPluginHandler, plugin);
let style =
gBrowser.contentWindow.getComputedStyle(elt("pleaseSubmit"));
is(style.display, "block", "Submission UI visibility should be correct");

elt("submitComment").value = "a test comment";
is(elt("submitURLOptIn").checked, true, "URL opt-in should default to true");
EventUtils.synthesizeMouseAtCenter(elt("submitURLOptIn"), {}, gTestBrowser.contentWindow);
EventUtils.synthesizeMouseAtCenter(elt("submitButton"), {}, gTestBrowser.contentWindow);
// And now wait for the submission status notification.
}
catch (err) {
failWithException(err);
}
}

I think instead of this, the content should have a frame script injected that detects onCrash, and then messages these values to the parent. Instead of listening for onCrash in the parent, we listen for the message, and then perform the test with what gets passed up to us. Ok, so we have a plan there.

browser_CTP_data_urls.js

So it's test3a that's failing out, because apparently:

let condition = function() objLoadingContent.activated;
waitForCondition(condition, test3b, "Test 3a, Waited too long for plugin to activate");

This condition never gets to true… hrm. So what's happening is that the plugin is actually _not_ activating.

Ah… I'm getting this in PluginContent.jsm in updateNotificationUI:

let contentWindow = this.global.content;
let cwu = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor)
.getInterface(Ci.nsIDOMWindowUtils);

contentWindow is null.

Stack to call that function is:

PluginContent.prototype.updateNotificationUI@resource:///modules/PluginContent.jsm:749:115
PluginContent.prototype.receiveMessage/<@resource:///modules/PluginContent.jsm:57:26

setTimeout_timer@resource://gre/modules/Timer.jsm:30:5

and that setTimeout timer is set in receiveMessage as:

case "BrowserPlugins:NotificationShown":
setTimeout(() => this.updateNotificationUI(), 0);
break;

So we've received a BrowserPlugins:NotificationShown, but content is null. How can that happen?

Where are PluginContent's constructed? How can it be constructed with a global that has no content?

Created in browser/base/content/content.js…

// TODO: Load this lazily so the JSM is run only if a relevant event/message fires.
let pluginContent = new PluginContent(global);

Waaaaait - this contentWindow stuff might be a red herring.

"Allow Now" and "Allow and Remember" are the panel buttons in test3a without my patch. But with my patch, it's "Block plugin" and "Continue Allowing".

So we end up clicking "Block plugin", when we should be clicking "Allow now". Ah hah. So behaviour has changed - something is clearly busted.

Where and when do we choose these two sets of options? urlbarBindings.xml seems to be where the panel is implemented.

The translation keys we want are pluginActivateNow.label and pluginActivateAlways.label, but what we got were pluginBlockNow.label and pluginContinue.label.

So my current patch makes us enter this block:

if (action.fallbackType == Ci.nsIObjectLoadingContent.PLUGIN_ACTIVE) {

When we should be entering the else down below. Confirm? Yes, that's exactly what's happening. For some reason, my patch makes it so that the front-end thinks that the plugin instance in the content process is already active.

Stack for entering _setupSingleState in bad case:

42 INFO _setupSingleState entered. _setupSingleState@chrome://browser/content/urlbarBindings.xml:1714:50
43 INFO _setState@chrome://browser/content/urlbarBindings.xml:1671:13
44 INFO click-to-play-plugins-notification_XBL_Constructor@chrome://browser/content/urlbarBindings.xml:1654:13
45 INFO PopupNotifications_refreshPanel/<@resource://gre/modules/PopupNotifications.jsm:597:7
46 INFO PopupNotifications_refreshPanel@resource://gre/modules/PopupNotifications.jsm:535:1
47 INFO PopupNotifications_showPanel@resource://gre/modules/PopupNotifications.jsm:617:5
48 INFO PopupNotifications_update@resource://gre/modules/PopupNotifications.jsm:699:7
49 INFO PopupNotifications_reshowNotifications@resource://gre/modules/PopupNotifications.jsm:812:5
50 INFO Notification.prototype.reshow@resource://gre/modules/PopupNotifications.jsm:93:5
51 INFO test3a@chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_data_urls.js:167:3
52 INFO testScope/test_executeSoon/<.run@chrome://mochikit/content/browser-test.js:805:9

Ok, showClickToPlayNotification seems to get the same bogus value for the Test plugin…

I think the content process is sending up the wrong data for the fallback type for the plugin. showClickToPlayNotification gets its information from the content process via the PluginContent:ShowClickToPlayNotification message.

Ah, ok - so it looks like the instance of PluginContent for the window holds on to pluginData, and that information doesn't get cleared. The lifetime of that data needs to be thought through more. It looks like pre-e10s ties the lifetime of this information to the lifetime of a notification… and I think it dies once the notification closes. Let's check.

Yep, that's the case. I've hooked up a listener for the notification removed event which sends a message to the child, which clears the cache. Test now passes.

browser_CTP_drag_drop.js

This is what we get when we run the test:

Stack trace:

chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js:part1:32

chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js:handleEvent:27

chrome://mozapps/content/plugins/pluginProblem.xml:pluginProblem_XBL_Constructor:83

null:null:0

TEST-INFO | expected PASS

60 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Waited too long for click-to-play notification -

Stack trace:

chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:waitForCondition/interval<:33

null:null:0

TEST-INFO | expected PASS

61 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Should have a click-to-play notification in the tab in the new window -

Stack trace:

chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js:part3:46

chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:waitForCondition/moveOn:48

chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:waitForCondition/interval<:34

null:null:0

TEST-INFO | expected PASS

62 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Waited too long for click-to-play notification -

Stack trace:

chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:waitForCondition/interval<:33

null:null:0

TEST-INFO | expected PASS

63 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Should have a click-to-play notification in the initial tab again -

Stack trace:

chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js:part4:56

chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:waitForCondition/moveOn:48

chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:waitForCondition/interval<:34

null:null:0

TEST-INFO | expected PASS

64 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Should have a click-to-play notification in the initial tab -

Stack trace:

chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js:part5:65

chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js:handleEvent:27

chrome://mozapps/content/plugins/pluginProblem.xml:pluginProblem_XBL_Constructor:83

null:null:0

TEST-INFO | expected PASS

65 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Waited too long for click-to-play notification -

Stack trace:

chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:waitForCondition/interval<:33

null:null:0

TEST-INFO | expected PASS

66 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | Should have a click-to-play notification in the tab in the new window -

Stack trace:

chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js:part7:77

chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:waitForCondition/moveOn:48

chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:waitForCondition/interval<:34

null:null:0

TEST-INFO | expected PASS

67 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js | plugin should be activated now -

Stack trace:

chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_drag_drop.js:part8:95

chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:waitForCondition/moveOn:48

chrome://mochitests/content/browser/browser/base/content/test/plugins/head.js:waitForCondition/interval<:44

null:null:0

TEST-INFO | expected PASS

What does this test even do?

Ok, so what we do is we create a tab with a plugin, and make sure we're showing a click-to-play notification. Then, we tear that tab out into a new window, and wait for a click-to-play notification. The notification is not showing up, so there's legit bustage here.

Actually, we don't even get the click-to-play notification in the first tab… what's going on there? No, wait that's showing just fine. Nevermind.

So, when working, the stack for re-showing the notification when dragging into a new window is:

PH_showClickToPlayNotification@chrome://browser/content/browser.js:4611:40

PH_reshowClickToPlayNotification@chrome://browser/content/browser.js:4467:5

pageShowEventHandlers@chrome://browser/content/browser.js:10121:5

Let's see which of those we hit with our patch…

TL;DR: Gecko isn't sending pageshow / pagehide events to content scripts! Filed bug 1058164 to get this fixed.

So I think pagehide / pageshow work fine when doing normal bfcache retrievals - it's just when swapping frameloaders where things go wrong. I've filed blocking bug 1058164 about that.

So I need to figure out how nsFrameLoader works, once and for all. How does it fit in? Does it exist in chrome as well as content processes? I guess it might, since content can have arbitrary subtrees of frames…

nsFrameLoader

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIFrameLoader

I think I'm going to put this exploration on hold until I've checked out the rest of the tests.

browser_CTP_notificationBar.js

So what's this guy's problem? It's strange… I think the pageshow clearing of the pluginData cache is wrong. I don't see anything like that going on before this patch. I think I'm going to excise it and see what happens.

Just need to async-ify some of these get notifications, and this one is fixed.

browser_bug743421.js

Looks like this one is attempting to reach into the document and read the state of the plugin. A content script should fix this.

PHWOAR - I've decided to tackle browser_CTP_crashreporting, and I injected the framescript as planned, but now I'm dealing with a leak on that test…

Ok, I've added an unload listener to PluginContent which destroys this.global and this.content, and that seems to have cleared up the leak. Phew.

browser_plugins_added_dynamically.js

WHAT IS YOUR MAJOR MALFUNCTION.

Oh, this is hilarious. It only fails if it runs with the entire folder - just like browser_bug820497.js. Lovely.

*sigh*, time to figure out where the interaction is. Innnnteresting - it looks like we're getting the same kind of issue as for browser_CTP_data_urls.js - the buttons that are coming up in the popup aren't the right ones for some reason.

I would wager that this is because we're not properly clearing out pluginData in PluginContent.jsm. Ah… so, clearing out pluginData on pageshow makes this work. But that breaks browser_CTP_notificationBar.js…

I really need to figure out this lifetime of pluginData inside PluginContent.jsm.

PluginData

What is it, any why is it, how does it work, and how do we make it work that way in e10s?

So, it appears to be a cache that is tied to the lifetime of a popup notification. We open a popup notification for CTP, and we populate it - if we notice that there's no pluginData cache, we create a new one and populate it.

It looks like a notification is only ever removed if we travel through the bfcache to a page that has no plugins at all.

PopupNotifications get dropped (ie, the removed event is fired) when changing locations. That's good - we should be clearing the pluginData in that case.

Ah! PopupNotifications don't fire a removed event when the tab their browser belongs to is closed!! That's what's going on here. Wait… we should be getting a new PluginContent.jsm per tab, so if we close a tab, we should destroy that PluginContent.jsm, and the new one should get… well, a new pluginData. Is that not what's happening here?

AH HAH - the pluginData Map in PluginContent.jsm was set on the prototype of PluginContent, meaning that it was shared across every instance!! I've made it so that the constructor gives each instance its own cache, as expected. Whew!

Back to nsFrameLoader

And then in his comment on the bug:

"That all is for non-e10s of course.
In case of e10s we should probably dispatch events to xul:browsers _and_ to TabChildGlobals"

So, we want to dispatch the pageshow / pagehide events to the window parents for non-e10s. For e10s, I have to wait until bug 918634 is fixed.

I've got the pageshow event now dispatching properly to the content script, and we're triggering things properly, and that's great. What's not so great is that I'm seeing errors like this when we try to open the CTP notification:

ERROR resource:///modules/PluginContent.jsm:742 - NS_ERROR_NOT_IMPLEMENTED: Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsIURI.userPass]

and that line 742 maps to this piece of code:

this.global.sendAsyncMessage("PluginContent:ShowClickToPlayNotification", {
plugins: [... this.pluginData.values()],
showNow: showNow,
principal: this._remotePrincipal(principal),
host: principalHost,
});

Something isn't getting cloned properly. The Principal is my top guess.

This bug, that bz fixed, introduced the frameloader swapping stuff: https://bugzilla.mozilla.org/show_bug.cgi?id=113934

ourEventTarget: mRawPtr     nsInProcessTabChildGlobal *     0x12cda33a0     0x000000012cda33a0

otherEventTarget: mRawPtr     nsInProcessTabChildGlobal *     0x11e1ed2c0     0x000000011e1ed2c0

OK! Figured it out - I explained in this comment and smaug confirmed.

ALL TESTS ARE NOW PASSING! \o/

Compare handleEvent in mbrubeck's PluginContent.jsm with the one in master

Just pass raw principals over the message manager instead of reconstituting them (see billm's comment)

Fix browser_CTP_crashreporting.js and browser_pluginCrashCommentAndURL.js by using the plan I mentioned above.

Fix nested iframe click-to-play regression (iframe in an iframe with flash doesn't get blocked!)

Remove any really gross dump statements / debugging crap so I can post a WIP.

Make sure I've truly got the lifetime of pluginData down properly.

Split up patch into smaller chunks

Evaluate the gross hacks I used to fix the tests and try to come up with a better plan.

What smaller chunks can I split the patch up into?

1. Changes to browser-plugins.js
2. Copy of browser-plugins.js to PluginContent.jsm, and changes therein.
3. Changes to tests.
4. Misc changes to browser/ stuff that doesn't fall into the above.

Ok, I've posted my patches, and targeted jaws. Hopefully it's not too much!

Hold up… I've been spending this whole time just making tests pass. Have I even ensured that CTP works for e10s, which is the whole point of this?

Yeah, it seems to work. WHEW. Jesus.

Relevant tests:

browser/base/content/test/plugins/browser_globalplugin_crashinfobar.js (fixed)

browser/base/content/test/plugins/browser_CTP_zoom.js (fixed)

browser/base/content/test/plugins/ browser_CTP_data_urls.js (fixed)

browser/base/content/test/plugins/ browser_CTP_crashreporting.js (fixed)

browser/base/content/test/plugins/ browser_pluginCrashCommentAndURL.js ("fixed" - this test needs a big cleanup though.)

browser/base/content/test/plugins/browser_CTP_drag_drop.js (fixed)

browser/base/content/test/plugins/browser_CTP_notificationBar.js (fixed)

browser/base/content/test/plugins/browser_bug743421.js (fixed)

browser/base/content/test/plugins/browser_bug820497.js (fixed)

browser/base/content/test/plugins/browser_globalplugin_crashinfobar.js (fixed)

browser/base/content/test/plugins/browser_pluginnotification.js (fixed)

browser/base/content/test/plugins/browser_pluginplaypreview.js (fixed)

browser/base/content/test/plugins/browser_pluginplaypreview2.js (fixed)

browser/base/content/test/plugins/browser_plugins_added_dynamically.js (fixed)

browser/base/content/test/plugins/browser_CTP_iframe.js (fixed - solution needs slight cleanup)

Test cleanup - second pass

Ok, now that I've got tests that pass, I more or less know that the CTP implementation is in pretty good shape.

Now what I need to do is clean-up these tests so that they're in better shape. I added some pretty hacky stuff to get them to pass. Let's see if we can make that better. I'll go from easiest to hardest.

browser_globalplugin_crashinfobar.js (easy - clean)

browser_bug820497.js (easy - clean)

browser_CTP_notificationBar.js (medium - clean)

browser_CTP_iframe.js (medium - clean)

browser_CTP_drag_drop.js (medium - clean)

browser_pluginCrashCommentAndURL.js (hard)

browser_CTP_crashreporting.js (hard)

So… pluginCrashCommentAndURL and crashreporting are two tests that follow a particular pattern - namely, they trigger crashes, then after the PluginCrash event fires, they manipulate the crashed plugin UI, and then listen for things in the crash submission notification and checks things in there.

I think we should probably redesign these tests a little bit.

Instead of having this external content script, maybe we just do the script-to-dataURI trick we used before, and forget about having some kind of specialized tool for testing content-y things. I think that's probably best.

Done my cleanup!

Here's a try push with everything:

https://tbpl.mozilla.org/?tree=Try&rev=7db87d3cf138

BAH - some failures in plugin tests that I didn't know about under mozapps. And what appears like a leak. :/

Let's do mozapps failures first.

toolkit/mozapps/extensions/test/browser/browser_CTP_plugins.js

toolkit/mozapps/extensions/test/browser/test-window/browser_CTP_plugins.js

Fixed! It was just a bunch of waiting for notifications again.

Now how about this leak …

Ooooh… and maybe an intermittent:

browser/base/content/test/plugins/browser_pluginnotification.js | Test 24b, plugin should be activated -

Guh… so, this leak…

WARNING -  TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_bug409481.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/web-panels.xul]

I'm disabling this test to see if the leak goes away… but I have my doubts that it'll go away. If it does, that's super interesting. I seem to be able to reproduce this on my Ubuntu machine, but only when I run bc1. :/

Guh. So hard to tell. Gonna try with a try push.

https://tbpl.mozilla.org/?tree=Try&rev=d6f7fe254ce3

https://tbpl.mozilla.org/?tree=Try&rev=58fc64c6c64d

Er… hrm, interesting. Passes for both. Accidentally disabled the test in the second as well. :/ But I didn't expect disabling the test to work. Strange.

https://tbpl.mozilla.org/?tree=Try&rev=9155d605db85

PluginContent removed. https://tbpl.mozilla.org/?tree=Try&rev=e119b3817067 - wow - Still leaking!!

What if we remove all of the test fixups and browser misc?: https://tbpl.mozilla.org/?tree=Try&rev=159a2fd63243 … LEAK

And what if we do _just_ test-fixups, but no CTP changes?: https://tbpl.mozilla.org/?tree=Try&rev=740201e35419 - GREEN.

So, the problem seems to be not in PluginContent, but either in the browser-plugins.js changes, or the misc stuff…

So it's starting to look like something is up in browser-plugins.js.

Maybe we need to deinit gPluginHandler, and remove the message manager listeners. https://tbpl.mozilla.org/?tree=Try&rev=6e28483c0614

YEAH, that fixed it. LEAK SOLVED, YO.

But now I see intermittent: browser/base/content/test/plugins/browser_pluginnotification.js | Test 24b, plugin should be activated -

Same as before. Let's look at that now. Might just be a matter of upping the timeout, or waiting for something.

Also, I need to respond to georg's feedback, and then convince him that he has review powers!

Ok, I think I have made browser_pluginnotification.js more robust. Let's see what this try push shows: https://tbpl.mozilla.org/?tree=Try&rev=9ba04fa90de9

Bunch-o-logging: https://tbpl.mozilla.org/?tree=Try&rev=893047d17b34

Next time I need to just do bc1, just try mochitest-browser-chrome-1.

Ok, let's try bumping the timeouts a bit: https://tbpl.mozilla.org/?tree=Try&rev=c4e1db8b62aa

Hm, now the whole test is timing out. I think perhaps the plugin just isn't being activated. What the hell? Maybe I should dump the label of the button that we're clicking to make sure we're actually doing the activation. And maybe put some dumps around the critical part of the test.

Even mooooore logging: https://tbpl.mozilla.org/?tree=Try&rev=86035c69fc5c

Ah hah - according to the logging, we've got "Allow Now" instead of "Allow Always" in the button that 24a is clicking. Strangely, it's also "Allow Now" in the passing case. What's gone wrong here?

Let's see what happens if I make us push the secondary button ("Allow and remember") instead of the primary?

https://tbpl.mozilla.org/?tree=Try&rev=7c9c76bcccaf

Grrr… still failing. What the hell?

What exactly is this test doing?

Hmm… nothing fishy there. Here's a screenshot of the test failure:

Yep, that's definitely not activated. Maybe even though the child is receiving the activation message, we're not actually activating ever. I've put a bunch of logging in PluginContent::activatePlugins to see if it's bailing out anywhere.

https://tbpl.mozilla.org/?tree=Try&rev=6975ab6313ed

Ok, instrumented nsObjectLoadingContent a bit… but now I've got a new theory. In test24a, we click on the panel button to activate the plugin, and then we immediately browse away. Y'know, I'll bet that plugin activation used to be synchronous. With PluginContent.jsm though, it's not - and I'll bet that's what's going wrong! We send the message to activate, but it never makes it to the child before we browse away! So we just need to wait for the plugin to activate before we browse away and trigger test24b. I've added a wait and pushed to try. FINGERS CROSSED!

https://tbpl.mozilla.org/?tree=Try&rev=3dad8255682c

Woo! I think that did it! Pushing to try for all platforms now.

https://tbpl.mozilla.org/?tree=Try&rev=f7d33907fdb4

Uh, whoops. Cruft in that patch.

https://tbpl.mozilla.org/?tree=Try&rev=f8d6ea64d3de

GRALKHAKSJHKJHASKRAJSLRASL it's still there. :(((

*sigh*. Back to the logging patch. This time, without the thing that caused all that orange.

https://tbpl.mozilla.org/?tree=Try&rev=2b4c99b8b5e9

Uh… now orange is gone. Wtf? Let's try removing all of my printf statements and some dump statements…

https://tbpl.mozilla.org/?tree=Try&rev=bca603395f47

Okayyyyy…. now it's gone again… cautiously push with the dump statements removed… but keep SimpleTest.requestCompleteLog() (wtf)

https://tbpl.mozilla.org/?tree=Try&rev=32fff51c2bf9

Oh great, now another intermittent orange:

509 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_data_urls.js | Test 3a, Waited too long for plugin to activate -
511 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_CTP_data_urls.js | Test 3c, Plugin should be activated -

Ok… time for more instrumentation *sigh*

Ok, instrumented it like crazy. Here goes.

https://tbpl.mozilla.org/?tree=Try&rev=1565014ccce7

Oooh - and I've got some reviews from Georg. Great! Nothing too serious it seems - fantastic.

Ah, interesting data from those logs. It looks like in the failing case, which is rare, the button that we're pressing in the popup is "Ok" and not "Allow now". Under what circumstances is the popup saying "OK"? Er… interesting. None of those buttons should be "OK"… and the request is sends down is "Block". Wtf?

Ah… that's button-accept from dialog.properties.

Well… we'll see how bad that intermittent orange gets. I _think_ I know what's up with it, anyhow.

Landed! Let's see if it sticks on fx-team. Fingers crossed.

Things to do:

File a follow-up for handling content process crashes to clean up notification bars - bug 1067495

File a follow-up to load PluginContent lazily - bug 1067497

GRRRR - bounced.

browser_pluginnotification.js is failing again, at the same spot. The good news is that I seem to be able to reproduce this locally with --run-until-failure.

I've added code that waits for the notification to be removed before continuing. Testing…

Hm, seems to be better, but still room for improvement.

Bad state:

Child is sending ShowClickToPlayNotification

Telling content process to clear caches

Parent process is showing click to play notification

Clearing plugin data cache

Child is sending ShowClickToPlayNotification

Parent process is showing click to play notification

3917 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, Should have a click-to-play notification

3918 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, Found plugin in page

3919 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, plugin fallback type should be PLUGIN_CLICK_TO_PLAY

3920 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, plugin should have started

3921 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, plugin should be activated

3922 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, plugin should be unloaded

3923 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, Plugin should not have activated

3924 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, Plugin should be click-to-play

3925 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, plugin node should not be activated

Child is sending ShowClickToPlayNotification

OHAI: removed

Telling content process to clear caches

Resolving

Parent process is showing click to play notification

Clearing plugin data cache

Child is sending ShowClickToPlayNotification

Parent process is showing click to play notification

Ok, running after binding attached - test24a...

3926 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24a, Should have a click-to-play notification

3927 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24a, Found plugin in page

3928 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24a, Plugin should be click-to-play

3929 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24a, plugin should not be activated

Clicking on Allow now..., right?: Allow Now

Child is sending ShowClickToPlayNotification

Parent process is showing click to play notification

Telling content process to clear caches

Clearing plugin data cache

Child is sending ShowClickToPlayNotification

Parent process is showing click to play notification

3930 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24b, Found plugin in page

3931 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24b, plugin should be activated -

Good state:

Child is sending ShowClickToPlayNotification

Telling content process to clear caches

Parent process is showing click to play notification

Clearing plugin data cache

Child is sending ShowClickToPlayNotification

Parent process is showing click to play notification

152 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, Should have a click-to-play notification

153 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, Found plugin in page

154 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, plugin fallback type should be PLUGIN_CLICK_TO_PLAY

155 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, plugin should have started

156 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, plugin should be activated

157 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, plugin should be unloaded

158 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, Plugin should not have activated

159 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, Plugin should be click-to-play

160 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 23, plugin node should not be activated

Child is sending ShowClickToPlayNotification

Parent process is showing click to play notification

OHAI: removed

Telling content process to clear caches

Resolving

Clearing plugin data cache

Child is sending ShowClickToPlayNotification

Parent process is showing click to play notification

Ok, running after binding attached - test24a...

161 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24a, Should have a click-to-play notification

162 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24a, Found plugin in page

163 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24a, Plugin should be click-to-play

164 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24a, plugin should not be activated

Clicking on Allow now..., right?: Allow Now

Child is sending ShowClickToPlayNotification

Parent process is showing click to play notification

Telling content process to clear caches

Clearing plugin data cache

Child is sending ShowClickToPlayNotification

165 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24b, Found plugin in page

Parent process is showing click to play notification

So I have a patch that works, but _why_ does it rely on the notification being reshown?

Bad state:

Setting permission for plugin:libnptest : 1 for expireType 1 in 1410919002828 ms.

Sending BrowserPlugins:ActivatePlugins

Handling activate plugins in child.

Playing plugin

Setting mActivated to true at checkpoint 3

Determining final type

Plugin type with no URI - skipping channel load

mActivated was not set to true because mActivated is true and mType is 2

updating notification UI in the child

Enabled state of plugin is: 1

Testing permission for plugin:libnptest - got 0

Shouldplay false - checkpoint 14

Showing overlay

Enabled state of plugin is: 1

Testing permission for plugin:libnptest - got 0

Shouldplay false - checkpoint 14

Child telling parent to show click to play notification.

Parent received message to show c2p notification.

FIRING REMOVED FROM CHECKPOINT 1

REMOVED

Determining final type

Plugin type with no URI - skipping channel load

Enabled state of plugin is: 1

Testing permission for plugin:libnptest - got 0

Shouldplay false - checkpoint 14

Nulling at checkpoint 3

mActivated was not set to true because mActivated is false and mType is 4

Nulling at checkpoint 11

Enabled state of plugin is: 1

Testing permission for plugin:libnptest - got 0

Shouldplay false - checkpoint 14

Enabled state of plugin is: 1

Testing permission for plugin:libnptest - got 0

Shouldplay false - checkpoint 14

Child telling parent to show click to play notification.

Parent received message to show c2p notification.

updating notification UI in the child

Enabled state of plugin is: 1

Testing permission for plugin:libnptest - got 0

Shouldplay false - checkpoint 14

Showing overlay

1878 INFO TEST-PASS | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24b, Found plugin in page

1879 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/plugins/browser_pluginnotification.js | Test 24b, plugin should be activated -

Good state:

Theory - Test 23 appends a node which causes a click to play message to come up from the child, and then test 24 starts to run. Test 23's notification is mapped to the system principal, and suddenly shows up, which we click, which …. plays Test 24a's plugin? Confirm?

And then when we reload, the system principal is not the one we want so 24b's plugin is not activated by default.

Ok, put together fix, got it reviewed, and re-landed. Here's hoping it sticks THIS TIME.

Ugh, slight problem.

Ok, landed patch for bug 1069567 to make the nsIPrincipal comparison work properly.