Bug 130694

Summary: Track plugin in visibility in the plugin process.
Product: WebKit Reporter: Stephanie Lewis <slewis>
Component: Plug-insAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, buildbot, commit-queue, rniwa, slewis
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Unspecified   
Attachments:
Description Flags
patch
andersca: review-, buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
none
take 2 andersca: review+

Description Stephanie Lewis 2014-03-24 12:45:26 PDT
Created attachment 227682 [details]
patch

Track plugin in visibility in the plugin process.  This allows the Plugin Process to handle its own app sleep assertions and sleep more often.
<rdar://problem/16061257> PluginProcess should AppNap when no plugins on active tab.
Comment 1 WebKit Commit Bot 2014-03-24 12:46:07 PDT
Attachment 227682 [details] did not pass style-queue:


ERROR: Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: Source/WebKit2/PluginProcess/WebProcessConnection.cpp:342:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 2 Build Bot 2014-03-24 13:44:34 PDT
Comment on attachment 227682 [details]
patch

Attachment 227682 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6113792429654016

New failing tests:
media/W3C/audio/canPlayType/canPlayType_application_octet_stream.html
Comment 3 Build Bot 2014-03-24 13:44:37 PDT
Created attachment 227688 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 4 Stephanie Lewis 2014-03-24 14:36:35 PDT
Layout test failures seem unrelated.  

Unexpected flakiness: text-only failures (2)
  fast/repaint/obscured-background-no-repaint.html [ Failure Pass ]
  transitions/3d/interrupted-transition.html [ Failure Pass ]


Regressions: Unexpected image-only failures (1)
  mathml/presentation/tokenElements-background-color.html [ ImageOnlyFailure ]

Regressions: Unexpected crashes (1)
  media/W3C/audio/canPlayType/canPlayType_application_octet_stream.html [ Crash ]

The crash is slowness in MathML

746 WebCore::RenderMathMLOperator::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&)  (in WebCore) + 915  [0x10ac45de3]  RenderMathMLOperator.cpp:1587
Comment 5 Stephanie Lewis 2014-03-24 19:31:01 PDT
Can't reproduce the layout test failures.
Comment 6 Anders Carlsson 2014-03-25 11:18:16 PDT
Comment on attachment 227682 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=227682&action=review

I don't think this handles the case where a web process crashes.

> Source/WebKit2/PluginProcess/PluginProcess.cpp:243
> +void PluginProcess::webProcessesWithVisiblePluginsChanged(bool isVisible)
> +{
> +    isVisible ? m_visiblePluginsActivity.increment() : m_visiblePluginsActivity.decrement();
> +}

I think having two functions, one for "didBecomeHidden" and one for "didBecomeVisible" is more clear.

> Source/WebKit2/PluginProcess/WebProcessConnection.cpp:342
> +    if (oldState != m_visiblePluginInstanceIDs.isEmpty()) {
> +        PluginProcess::shared().webProcessesWithVisiblePluginsChanged(isVisible);
> +    }

Single line if bodies shouldn't have braces.
Comment 7 Stephanie Lewis 2014-03-25 14:57:21 PDT
Created attachment 227803 [details]
take 2

The PluginProcess does the right  thing if the WebProcess crashes.  A cleanup function on the WebConnection is called that destroys all the pluginControllers -> who remove themselves from the connection -> which cleans up the visible plugins, and eventually the connection is destroyed altogether.
Comment 8 WebKit Commit Bot 2014-03-25 14:58:58 PDT
Attachment 227803 [details] did not pass style-queue:


ERROR: Source/WebKit2/PluginProcess/PluginProcess.cpp:239:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Anders Carlsson 2014-03-28 14:17:31 PDT
Comment on attachment 227803 [details]
take 2

View in context: https://bugs.webkit.org/attachment.cgi?id=227803&action=review

> Source/WebKit2/PluginProcess/WebProcessConnection.cpp:331
> +    bool oldState = m_visiblePluginInstanceIDs.isEmpty();

Can you ASSERT(!m_visiblePluginInstanceIDs.contains(pluginInstanceID))?

> Source/WebKit2/PluginProcess/WebProcessConnection.cpp:346
> +    if (m_visiblePluginInstanceIDs.contains(pluginInstanceID))
> +        m_visiblePluginInstanceIDs.remove(pluginInstanceID);

remove is just a no-op if the set doesn't contain the plug-in instance so there's no need to do a .contains check here.

Can you assert that m_visiblePluginInstanceIDs.contains(pluginInstanceID) always returns true here?

> Source/WebKit2/PluginProcess/WebProcessConnection.h:58
> +    void pluginDidBecomeVisible(unsigned pluginInstanceId);
> +    void pluginDidBecomeHidden(unsigned pluginInstanceId);

Capital d in ID.

> Source/WebKit2/PluginProcess/mac/PluginControllerProxyMac.mm:109
> +    printf("Visibility changed  to %d for plugin %llu from %d\n", isVisible, m_pluginInstanceID, getpid());

Please remove this.

> Source/WebKit2/PluginProcess/mac/PluginControllerProxyMac.mm:111
> +    isVisible ? m_connection->pluginDidBecomeVisible(m_pluginInstanceID) : m_connection->pluginDidBecomeHidden(m_pluginInstanceID);

I think an if statement is cleaner than a ternary operator here.
Comment 10 Stephanie Lewis 2014-03-28 16:25:20 PDT
(In reply to comment #9)
> (From update of attachment 227803 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=227803&action=review
> 
> > Source/WebKit2/PluginProcess/WebProcessConnection.cpp:331
> > +    bool oldState = m_visiblePluginInstanceIDs.isEmpty();
> 
> Can you ASSERT(!m_visiblePluginInstanceIDs.contains(pluginInstanceID))?
> 

It's valid to receive a plugin did hide message for a plugin we don't track as visible.

We get a plugin visibility changed message shortly after the plugin has been initialized which could be visible or hidden.
Comment 11 Stephanie Lewis 2014-03-28 18:20:06 PDT
Landed http://trac.webkit.org/changeset/166441