Bug 136197 – DOM timer throttling for hidden plugins

• Component: Plug-ins
• Status: RESOLVED FIXED

[Gavin Barraclough]

For non-visible web pages we slow timers (since they can't be driving animations, etc). We should do the same for plugins.

[Gavin Barraclough]

Created attachment 237044 [details]
Fix

[WebKit Commit Bot]

Attachment 237044 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/js/ScheduledAction.h:78:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 237044 [details]
Fix

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

The PlugIn vs. Plugin spelling thing is starting to annoy me.

review- because this doesn’t yet compile on non-Mac

> Source/WebCore/bindings/js/JSPluginElementFunctions.cpp:90
> +    if (Page* page = pluginElement.document().page()) {
> +        for (auto executingScheduledAction : page->executingScheduledActions())
> +            executingScheduledAction->scriptDidInteractWithPlugin(pluginElement);
> +    }

I suggest we move this code into a class member function of ScheduledAction that takes an HTMLPlugInElement. It would be nice to concentrate the knowledge of the details of this mechanism more thoroughly in ScheduledAction rather than distributing the code across classes.

This should be auto* or auto& instead of auto so it’s clear to the reader that there is no copying happening here.

> Source/WebCore/bindings/js/ScheduledAction.cpp:135
> +    if (executionRecord.didInvisibleInaudibleScriptInteractWithPlugin())
> +        m_shouldThrottle = true;

I’m confused about the rule here. The rule here is that if the script interacts with an invisible and inaudible plug-in it gets throttled. That seems a little backwards to me. What if it interacts with both visible plug-ins and invisible ones? This will cause that sort of script to be throttled. I think we need a why comment explaining the rule here since the rule doesn’t explain itself.

The rule this code currently implements is a heuristic in multiple ways. It’s based on fetching the plug-in object for use by script; I’m not entirely clear what operation that corresponds to.

It’s not that far from a global rule. It seems we could implement almost the same rule without all the data structures. Just a number that is bumped whenever we interact with these invisible inaudible plug-ins and then this function could fetch that number before and after. If it has changed, then set m_shouldThrottle to true. Really unsure what data structure is needed here.

> Source/WebCore/bindings/js/ScheduledAction.cpp:167
> +    ExecutingScheduledActionRecord* result = m_page->popExecutingScheduledAction();

I like auto* in places like this.

> Source/WebCore/bindings/js/ScheduledAction.cpp:174
> +    Widget* widget = pluginElement.pluginWidget();
> +    if (widget && !widget->isVisible() && widget->isPluginViewBase() && toPluginViewBase(widget)->audioHardwareActivity() == AudioHardwareActivityType::IsInactive)

Should this entire predicate be a member of the HTMLPlugInElement class? Maybe named isInvisibleAndInaudible? It’s a little ugly to have this digging around in the plug-in widget be here rather than in HTMLPlugInElement itself. But on the other hand, it’s also maybe nice not to have to touch HTMLPlugInElement.

Perhaps the PluginViewBase function should be a new isInaudible function rather than exposing the existing audioHardwareActivity function. Currently, the audioHardwareActivity function for plug-ins is entirely a concept local to WebKit2; it’s a little awkward to have it poke out into PluginViewBase like this, but I can’t think of anything better.

> Source/WebCore/bindings/js/ScheduledAction.cpp:175
> +        m_invisibleInaudibleScriptDidInteractWithPlugin = true;

Is it the script or the plug-in that is invisible and inaudible?

> Source/WebCore/bindings/js/ScheduledAction.h:78
> +    class ExecutingScheduledActionRecord
> +    {

Not sure the name “record” is quite the name for this programming pattern.

Our coding style puts the brace on the same line as the class name.

Normally for this we would use WTF_NONCOPYABLE since it’s not intrinsically not copyable (no references) but we wouldn’t correctly handle copying it. If we change this to hold a Ref<MainFrame> instead of a Page*, that won’t be necessary.

If you take my suggestion about concentrating the code more in ScheduleAction, we should be able to keep this class outside of the header entirely.

> Source/WebCore/bindings/js/ScheduledAction.h:87
> +        Page* m_page;

What guarantees that this pointer won’t outlive the Page? I don’t think anything does! We could use MainFrame instead; that’s something we can ref().

> Source/WebCore/page/DOMTimer.cpp:49
> +static const int minIntervalForThrottledScheduledAction = 1000;

Need a “why” comment somewhere about why this is an appropriate value.

> Source/WebCore/page/Page.h:110
> +typedef Vector<ExecutingScheduledActionRecord*, 2> ExecutingScheduledActionStack;

I don’t think we should have a typedef for this. We should not need to utter the type more than once.

> Source/WebCore/page/Page.h:437
> +    void pushExecutingScheduledAction(ExecutingScheduledActionRecord* record)

How about we use a single global Vector for this in ScheduledAction? It seems a shame to have to put these things into Page.h. I think we could make ScheduledAction do this without involving the page. One downside is that we’d have to iterate over all of these comparing page pointers (or probably main frame pointers if you take my advice above), but I am not sure this is a problem. Being O(all scripts executing) does not seem significantly worse than O(scripts executing in this page).

> Source/WebCore/page/Page.h:447
> +        ASSERT(m_executingScheduledActions.size());
> +        ExecutingScheduledActionRecord* result = m_executingScheduledActions.last();
> +        m_executingScheduledActions.removeLast();
> +        return result;

Vector already has all of this code, including the assert, in fact it’s a RELEASE_ASSERT. We should just write:

    return m_executingScheduledActions.takeLast();

> Source/WebCore/page/Page.h:450
> +    const ExecutingScheduledActionStack executingScheduledActions() const { return m_executingScheduledActions; }

The type here should be const auto&; we don’t want to copy the stack when getting it. Or do we? If we do, I think we should name the function something different.

> Source/WebCore/page/Page.h:612
> +    ExecutingScheduledActionStack m_executingScheduledActions;

Could consider a singly linked list instead of a vector, since we never iterate the vector without also visiting each of the items in it.

> Source/WebCore/plugins/PluginViewBase.h:78
> +    virtual AudioHardwareActivityType audioHardwareActivity() const { return AudioHardwareActivityType::Unknown; }

The override of this in PluginView.h should get a the virtual keyword and override keyword on it. I see how to do this for the WebKit2 version, but not for the version in WebCore itself. It seems, in fact, that we would want this be pure virtual. Unless perhaps this is simply “only implemented for WebKit2 at this time”?

I suggest removing the includes of AudioHardwareListener.h elsewhere that are no longer needed now that we have added this.

[Gavin Barraclough]

(In reply to comment #3)

Just commenting on select issues – more of the code issues should be addressed by a new patch.

> The PlugIn vs. Plugin spelling thing is starting to annoy me.

Me too. :-( I did a really quick audit to see if I could easily resolve, it's about a 3:1 split, so needed some closer inspection to see which way we should go.

> The rule this code currently implements is a heuristic in multiple ways. It’s based on fetching the plug-in object for use by script; I’m not entirely clear what operation that corresponds to.

This was a choke point suggested by Geoff as a good place to detect all types of script/plugin interaction (property access and method invocation).

> It’s not that far from a global rule. It seems we could implement almost the same rule without all the data structures. Just a number that is bumped whenever we interact with these invisible inaudible plug-ins and then this function could fetch that number before and after. If it has changed, then set m_shouldThrottle to true. Really unsure what data structure is needed here.

A single global int, checked before & after timer fire is exactly how my initial implementation works. The additional complexity of the current patch is unfortunate. The global made Sam sad, and it is a bogus that a script interaction on one page would interfere with state affecting another (the way it is used this wouldn't currently be a problem, but it seems too fragile). The simple implementation also leaves more details of the policy scattered across more of the code base; the patch as attached does do a better job of encapsulating this.

> Perhaps the PluginViewBase function should be a new isInaudible function rather than exposing the existing audioHardwareActivity function. Currently, the audioHardwareActivity function for plug-ins is entirely a concept local to WebKit2; it’s a little awkward to have it poke out into PluginViewBase like this, but I can’t think of anything better.

I'm inclined to keep the ternary result here; in this case we want to distinguish inaudible from audible/unknown, but in other uses one might need distinguish audible from inaudible/unknown.

> > Source/WebCore/bindings/js/ScheduledAction.h:87
> > +        Page* m_page;
> 
> What guarantees that this pointer won’t outlive the Page? I don’t think anything does! We could use MainFrame instead; that’s something we can ref().

Huh, I'd assumed the Page would be alive while DOMTimers on the page are firing, but it's probably better to be robust here. Will fix.

> > Source/WebCore/page/Page.h:612
> > +    ExecutingScheduledActionStack m_executingScheduledActions;
> 
> Could consider a singly linked list instead of a vector, since we never iterate the vector without also visiting each of the items in it.

I'd considered a doubly linked list to allow randomly ordered removal from the set; this way I could make the set of listeners more generic and not only applicable to the ScheduledActions (which I could assume would behave as a stack). I'll take another look at this.

[Gavin Barraclough]

Transmitting file data ....
Committed revision 173208.

[Gavin Barraclough]

Ooops, ignore last comment, wrong bug!

[Gavin Barraclough]

Created attachment 237585 [details]
New fix

[WebKit Commit Bot]

Attachment 237585 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMTimer.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Comment on attachment 237585 [details]
New fix

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

Windows build is bust:
    1>C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\audio\AudioHardwareListener.h(35): error C2365: 'Unknown' : redefinition; previous definition was 'enumerator' (..\inspector\InspectorAllInOne.cpp)

> Source/WebCore/page/DOMTimer.cpp:279
> +    // Apply two thottles - the global (per Page) minimum, and also a per-timer throttle.

thottles -> throttles

[Andreas Kling]

Comment on attachment 237585 [details]
New fix

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

> Source/WebCore/page/DOMTimer.cpp:68
> +        if (shouldSetCurrent)
> +            current = nullptr;

This will clear out the current fire-state prematurely if we are inside a nested timer fire.
If we had a stack of fire-states, we could laugh all the way to the bank.

[Geoffrey Garen]

Comment on attachment 237585 [details]
New fix

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

r=me, some suggested tweaks below.

> Source/WebCore/ChangeLog:14
> +            - DOMTimer::fired detect timers that interact with invisible/inaudible plugins, and flags itself for throtting.

"detects"

> Source/WebCore/html/HTMLPlugInElement.h:91
> +    bool isDetectable() const;

Maybe "isUserObservable"? "isDetectable" doesn't say to whom. Something about user seems good here.

> Source/WebCore/page/DOMTimer.cpp:51
> +static const int minIntervalForUndetectablePluginScriptTimers = 1000; // Empirically determined.

Empirically determined... to maximize battery life?

> Source/WebCore/page/DOMTimer.cpp:165
> +    // For worked threads, don't update the current DOMTimerFireState.

"worker"

Should say why: it's both not thread-safe, and not plugin-relevant.

> Source/WebCore/page/DOMTimer.cpp:166
> +    DOMTimerFireState fireState(context->isDocument());

Seems like it might be cleaner to pass the context to the constructor, rather than a boolean. It's an implementation detail of DOMTimerFireState that it only wants to track main thread / DOM timers.

> Source/WebCore/page/DOMTimer.cpp:279
> +    // Apply two thottles - the global (per Page) minimum, and also a per-timer throttle.

"throttles"

[Gavin Barraclough]

Created attachment 237598 [details]
Fixes for Andreas & Geoff's review comments; speculative windows build fix.

[WebKit Commit Bot]

Attachment 237598 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMTimer.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 237585 [details]
New fix

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

New failing tests:
platform/mac-wk2/plugins/asynchronous-destroy-before-initialization.html

[Build Bot]

Created attachment 237614 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 237598 [details]
Fixes for Andreas & Geoff's review comments; speculative windows build fix.

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

New failing tests:
platform/mac-wk2/plugins/asynchronous-destroy-before-initialization.html

[Build Bot]

Created attachment 237618 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Gavin Barraclough]

Created attachment 237726 [details]
Speculative gtk fix

[WebKit Commit Bot]

Attachment 237726 [details] did not pass style-queue:


ERROR: Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1753:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/page/DOMTimer.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Created attachment 237794 [details]
Hmmm

[WebKit Commit Bot]

Attachment 237794 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMTimer.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Created attachment 237813 [details]
Take 2(ish) at gtk fix.

[Gavin Barraclough]

Created attachment 237897 [details]
wtf

[WebKit Commit Bot]

Attachment 237897 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMTimer.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Created attachment 237903 [details]
ah.

[WebKit Commit Bot]

Attachment 237903 [details] did not pass style-queue:


ERROR: Source/WebCore/page/DOMTimer.h:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gavin Barraclough]

Committed revision 173694.