Closed
Bug 1245545
Opened 9 years ago
Closed 9 years ago
Ensure that dom.ipc.plugins.unloadTimeoutSecs works properly with e10s enabled (need automated test)
Categories
(Core Graveyard :: Plug-ins, defect)
Core Graveyard
Plug-ins
Tracking
(e10sm9+, firefox46 wontfix, firefox47 fixed)
RESOLVED
FIXED
mozilla47
People
(Reporter: benjamin, Assigned: qdot)
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(kyle)
Flags: needinfo?(aklotz)
Comment 1•9 years ago
|
||
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)
Assignee | ||
Comment 2•9 years ago
|
||
I'll try to take a look at it.
Assignee: nobody → kyle
Flags: needinfo?(kyle)
Updated•9 years ago
|
Assignee | ||
Comment 3•9 years ago
|
||
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.
Assignee | ||
Comment 4•9 years ago
|
||
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.
Assignee | ||
Comment 5•9 years ago
|
||
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)
Reporter | ||
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
(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)
Assignee | ||
Comment 8•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Attachment #8719910 -
Attachment mime type: text/x-log → text/plain
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Comment 10•9 years ago
|
||
Updated test to check both content and parent process for plugin unloading.
Attachment #8718130 -
Attachment is obsolete: true
Attachment #8721502 -
Flags: review?(benjamin)
Reporter | ||
Comment 11•9 years ago
|
||
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+
Comment 12•9 years ago
|
||
Comment 13•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 14•9 years ago
|
||
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.
status-firefox46:
--- → ?
Flags: needinfo?(kyle)
Assignee | ||
Comment 15•9 years ago
|
||
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)
Comment 16•9 years ago
|
||
I see. In that case, we don't need to uplift because a test-only change won't affect our e10s beta experiment.
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
•