Closed Bug 1245545 Opened 6 years ago Closed 6 years ago

Ensure that dom.ipc.plugins.unloadTimeoutSecs works properly with e10s enabled (need automated test)

Categories

(Core :: Plug-ins, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
e10s m9+ ---
firefox46 --- wontfix
firefox47 --- fixed

People

(Reporter: benjamin, Assigned: qdot)

Details

Attachments

(2 files, 2 obsolete files)

dom.ipc.plugins.unloadTimeoutSecs kills off a plugin N seconds after there are no active instances. By default this is 30 seconds, and this is valuable both for rarely-used plugins such as Java as well as commonly-used Flash so that we don't keep memory leaks around for long periods of time.

Reading the code, I expect that this setting just doesn't work in e10s at all, and so we'd never kill off plugin processes in e10s mode. But we need to have an automated test for this and then fix the e10s case if necessary.

qdot or aklotz, are either of you interested in investigating an automated test for this?
Flags: needinfo?(kyle)
Flags: needinfo?(aklotz)
I'm pretty tied up at the moment with sorting out the 44 stability situation. I can get to this if necessary but probably not right away.
Flags: needinfo?(aklotz)
I'll try to take a look at it.
Assignee: nobody → kyle
Flags: needinfo?(kyle)
Quick proof of concept test for verifying bug. Sure enough, this test works fine non-e10s, breaks on e10s. Not real happy about the use of setTimeout (could possibly poll? Just don't want to add any more than minimal state checking to nsPluginTag), but it'll work for now while I figure out a fix.
The problem seems to be that nsObjectLoadingContent in the child process isn't actually the instance owner for the plugin instance, so while nsObjectLoadingContent::StopPluginInstance is called in the child process, it stops there, not getting relayed to the parent.
Realizing there may be some weirdness with having the chrome process dealing with the parent process instead of the child, I changed this to a regular mochitest and it actually seems to work? All tests pass on both non-e10s and e10s, and the NSPR logs make it look like things are destructing correctly.

Not real sure my polling strategy for the unload is a good idea, as that's going to lead to failure via timeout only. Is there a better way of doing that in mochitest?
Attachment #8717668 - Attachment is obsolete: true
Attachment #8718130 - Flags: review?(benjamin)
Comment on attachment 8718130 [details] [diff] [review]
Patch 1 (v2) - Mochitest for plugin.unloadTimeoutSecs

In the e10s case, this test is running in the content process. Wouldn't that always be running the nsFakePluginTag?

What really matters is whether the chrome process has unloaded the actual plugin (tearing down the plugin process). The content process version of this doesn't matter, right?

What does the NSPR log say about the plugin process shutting down?
Flags: needinfo?(kyle)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> Comment on attachment 8718130 [details] [diff] [review]
> Patch 1 (v2) - Mochitest for plugin.unloadTimeoutSecs
> 
> In the e10s case, this test is running in the content process. Wouldn't that
> always be running the nsFakePluginTag?

Weirdly enough, no, because the test passes, and nsFakePluginTag always returns true for the "loaded" state, which would fail it. The content process has a nsPluginTag that does change state, but it's not unloading the library, because...

> What really matters is whether the chrome process has unloaded the actual
> plugin (tearing down the plugin process). The content process version of
> this doesn't matter, right?

The library in unloaded on the following chain of events:

- plugin iframe is removed in test
- PluginModuleChild::ActorDestroy is called, which sends NotifyContentModuleDestroyed to the parent
- In the parent, NotifyContentModuleDestroyed creates a nsPluginUnloadRunnable, which when run unloads the library.

So, from what I understand of what I'm seeing, the library does get unloaded (see attached log). However, you're right in that my test isn't actually checking for that, it's still checking for status in the child process, which is bogus (Also shows up in attached log, since the test passes before the parent unloads). I'll get that fixed.
Flags: needinfo?(kyle)
Attachment #8719910 - Attachment mime type: text/x-log → text/plain
Comment on attachment 8718130 [details] [diff] [review]
Patch 1 (v2) - Mochitest for plugin.unloadTimeoutSecs

Need to fix test to check for library load status in the parent process
Attachment #8718130 - Flags: review?(benjamin)
Updated test to check both content and parent process for plugin unloading.
Attachment #8718130 - Attachment is obsolete: true
Attachment #8721502 - Flags: review?(benjamin)
Comment on attachment 8721502 [details] [diff] [review]
Patch 1 (v3) - Mochitest for plugin.unloadTimeoutSecs

Aha, I totally missed the logic around mContentProcessRunningCount

Thanks!
Attachment #8721502 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/1ed4d0e86f7e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Kyle, should we uplift this plugin timeout fix to Beta 46? We're running an e10s telemetry experiment, so we would like to include fixes for important e10s issues.
Flags: needinfo?(kyle)
Well, there wasn't a fix, really. It was just a test to make sure things were working correctly, which they were. So, since it's a a=TEST-ONLY, I think we can uplift automatically if you want?
Flags: needinfo?(kyle)
I see. In that case, we don't need to uplift because a test-only change won't affect our e10s beta experiment.
You need to log in before you can comment on or make changes to this bug.