Bug 80428 – HTMLPluginElement is not destroyed on reload or navigation if getNPObject is called

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Dave Michael]

This bug has been spotted before, but the fix didn't cover all cases:
https://bugs.webkit.org/show_bug.cgi?id=66181

HTMLPluginElement::getNPObject looks like this:
"""
    ASSERT(document()->frame());
    if (!m_NPObject)
        m_NPObject = document()->frame()->script()->createScriptObjectForPluginElement(this);
    return m_NPObject;
"""
After this point, the NPObject (via the relevant JavaScript bindings) holds a reference on the HTMLPluginElement, and the HTMLPluginElement also owns a reference on the NPObject that wraps it. This is a circular reference, so without intervention, the HTMLPluginElement will never be destroyed.

(Probably, getNPObject should have just passed its NPObject reference to callers to avoid ever having. However, changing this behavior is probably too disruptive to existing code).

The patch in:
https://bugs.webkit.org/show_bug.cgi?id=66181
added:
"""
void HTMLPlugInElement::removedFromDocument()
{
#if ENABLE(NETSCAPE_PLUGIN_API)
    printf("removedFromDocument; m_NPObject: %p.\n", m_NPObject);
    if (m_NPObject) {
        _NPN_ReleaseObject(m_NPObject);
        m_NPObject = 0;
    }
#endif

    HTMLFrameOwnerElement::removedFromDocument();
}
"""
This way, when the plugin element is removed from the document, the circular reference is cleared and the HTMLPluginElement can be destroyed.

However, since removedFromDocument is not run on reload (nor, I think, on navigation), the HTMLPluginElement is still left in memory on reload or navigation. This tends to retain the associated Document object in memory.

I think the right approach (which wez suggested in his previous bug) is to release m_NPObject in detach(). I'll upload a patch soon, though I'm still struggling with coming up with a good way to write an automated test to reproduce this.

[Dave Michael]

Created attachment 130409 [details]
Patch

[Dave Michael]

Any tips on how to make an automated test for this would be appreciated. I don't think there's a way to do it with a layout test. I've been told there's a leak checker bot that might catch this if I construct a test that reproduces the leak, but I think that all the memory is still reachable, so I don't think a leak detection tool will see it.

[Dave Michael]

I've been struggling to find a way to test this. It's difficult for a few reasons:
- The PluginView is properly destroyed, and the plugin is thus "Deallocated" properly.
- All NPObjects are (apparently) force-released on Frame teardown, at which point the HTMLPluginElement will also be deleted. As a result, this doesn't show up as a leaked WebCoreNode

Due to these, there doesn't seem to be a way to test this using a layout test. (However, a lot of memory is wasted in practice while the process is running, because the Document that owns the HTMLPluginElement must persist as long as any of its children do. In Chromium, this can waste close to 1MB per reload for a very simple page.)

I think a test is possible if I do it as some kind of a unit test, where I have access to the internal/native WebKit classes. Then I can think of at least a couple of options:
(a) expose the Document weak pointer for use in the test. Then, I can:
  1) grab a weak pointer to the Document
  2) force a reload
  3) force garbage collection
  4) And ensure that the weak pointer's target document has been deallocated.
(b) Add Node's "liveNodeSet" to the debug build for the sake of my test. Then, add a "bool IsLiveNode(const Node*)" function, and:
  1) Save the raw pointer to the HTMLPluginElement
  2) force a reload
  3) force garbage collection
  4) Ensure that the raw pointer from 1) is not a "live" Node anymore.

However... this all sounds like a lot of work with some downsides (exposing currently-private data in (a), affecting performance of debug builds in (b)).

Would it make sense to maybe do this patch without a test? Or is there a better way to test it that I haven't thought of?

[Hajime Morrita]

Comment on attachment 130409 [details]
Patch

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

r- due to the format issue.

I don't try this by myself. but it looks the test plugin has a "logDestroy" attribute which we can emit a log for plugin destruction with.
See LayoutTests/plugins/resources/geturlnotify-on-destroy.html for example.

> Source/WebCore/ChangeLog:1
> [phone]  Dave Michael  <[e-mail]>

Please follow the format:

        Single line bug summary from Bugzilla
        https://bugs.webkit.org/show_bug.cgi?id=xxxx

        Reviewed by...

        Then long explanation follows....

[Dave Michael]

Created attachment 132057 [details]
Patch

[Dave Michael]

(In reply to comment #4)
> (From update of attachment 130409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130409&action=review
> 
> r- due to the format issue.
> 
> I don't try this by myself. but it looks the test plugin has a "logDestroy" attribute which we can emit a log for plugin destruction with.
> See LayoutTests/plugins/resources/geturlnotify-on-destroy.html for example.
I can do that, but unfortunately it will not exercise this bug. The render tree is getting destroyed properly, so the PluginView still calls "PluginPackage::unload", and that's the part of the code that tells the plugin to "Deallocate". So from the plugin's perspective, everything is working fine.

What is being improperly retained is the HTMLPluginElement itself in the DOM tree (and therefore its owning Document stays in memory as well).

I mentioned a couple of ideas above for possibly testing this by checking that the HTMLPluginElement Node (or Document) stays in memory longer than expected. But these approaches have drawbacks. Alternatively, it looks like the WebInspector also has a count of Nodes. If I can reach that from a Layout test, I might be able to test the problem that way.

> 
> > Source/WebCore/ChangeLog:1
> > [phone]  Dave Michael  <[e-mail]>
> 
> Please follow the format:
> 
>         Single line bug summary from Bugzilla
>         https://bugs.webkit.org/show_bug.cgi?id=xxxx
> 
>         Reviewed by...
> 
>         Then long explanation follows....

[Dave Michael]

Comment on attachment 132057 [details]
Patch

Thanks for the review. I updated the changelog and tried to explain why there's no test.

[Hajime Morrita]

(In reply to comment #7)
> (From update of attachment 132057 [details])
> Thanks for the review. I updated the changelog and tried to explain why there's no test.
Got the point. I was confused.

[Eric Seidel (no email)]

Comment on attachment 132057 [details]
Patch

Since we do not believe it possible to write an automated test, I think it would make sense to add a manual test.  My understanding is a very very simple page should trigger this.

Your manual test would go in "ManualTests" (in the root of the repository) and would basically instruct the user to reload the page repeatedly, and see if memory usage increases.

Please re-post wiht a manual test, and I'm ahppy to r+.

[Dave Michael]

Created attachment 132963 [details]
Patch

[Philippe Normand]

Comment on attachment 132963 [details]
Patch

Attachment 132963 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12072144

[Build Bot]

Comment on attachment 132963 [details]
Patch

Attachment 132963 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12066170

[Dave Michael]

Created attachment 132967 [details]
Patch

[Dave Michael]

Comment on attachment 132967 [details]
Patch

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

eseidel: Thanks for the suggestion of adding capabilities to window.internals. This turned out to be easier than expected, so here's my first stab at a test using this. I basically just reload and make sure that there's only 1 document.

> LayoutTests/plugins/netscape-dom-access-and-reload.html:36
> +        if (numberOfLiveDocuments == 1)

I'm worried about ports that don't build WebInspector (do we have any? Mobile stuff, or headless ports maybe?)

In those cases, this test isn't really valid, and numberOfLiveDocuments will be 0. What's the best way of dealing with that?

[Build Bot]

Comment on attachment 132967 [details]
Patch

Attachment 132967 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12066180

[Build Bot]

Comment on attachment 132967 [details]
Patch

Attachment 132967 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12072159

[Philippe Normand]

Comment on attachment 132967 [details]
Patch

Attachment 132967 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12066204

[Adam Barth]

Comment on attachment 132967 [details]
Patch

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

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:36
>> +        if (numberOfLiveDocuments == 1)
> 
> I'm worried about ports that don't build WebInspector (do we have any? Mobile stuff, or headless ports maybe?)
> 
> In those cases, this test isn't really valid, and numberOfLiveDocuments will be 0. What's the best way of dealing with that?

That's fine.  Those ports (if they exist) can skip this test.  Perhaps we should show a helpful message if numberOfLiveDocuments == 0 letting folks know that this might be caused by having the inspector disabled?

[Dave Michael]

Created attachment 133088 [details]
Patch

[Build Bot]

Comment on attachment 133088 [details]
Patch

Attachment 133088 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12071570

[Build Bot]

Comment on attachment 133088 [details]
Patch

Attachment 133088 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12090539

[Philippe Normand]

Comment on attachment 133088 [details]
Patch

Attachment 133088 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12072595

[Dave Michael]

Created attachment 133118 [details]
Patch

[Dave Michael]

+dglazkov, caseq: Can one of you guys help me figure out how to get this to build properly? I'm trying to use InspectorCounters::counterValue in testing/Internals.cpp, and I'm getting link errors. I might have mac sorted out (I added the symbol to WebCore.exp.in), but I'm not sure where in WebKit to tell the Windows build to export that symbol. GTK is also failing to build (though it's not clear to me if that's related to my patch)

[Build Bot]

Comment on attachment 133118 [details]
Patch

Attachment 133118 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12066633

[Dave Michael]

Created attachment 133132 [details]
Patch

[Philippe Normand]

./.libs/libWebCoreInternals.a(./.libs/../Source/WebCore/testing/.libs/libWebCoreInternals_la-Internals.o):Internals.cpp:function WebCore::Internals::numberOfLiveNodes() const: error: undefined reference to 'WebCore::InspectorCounters::counterValue(WebCore::InspectorCounters::CounterType)'
./.libs/libWebCoreInternals.a(./.libs/../Source/WebCore/testing/.libs/libWebCoreInternals_la-Internals.o):Internals.cpp:function WebCore::Internals::numberOfLiveDocuments() const: error: undefined reference to 'WebCore::InspectorCounters::counterValue(WebCore::InspectorCounters::CounterType)'
collect2: ld returned 1 exit status

You need to add those 2 symbols in Source/autotools/symbols.filter

[Gustavo Noronha (kov)]

Comment on attachment 133132 [details]
Patch

Attachment 133132 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12072631

[Dave Michael]

Created attachment 133170 [details]
Patch

[Dave Michael]

Comment on attachment 133170 [details]
Patch

I think I have all the right incantations to make this build on all the platforms.

[Ryosuke Niwa]

Comment on attachment 133170 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description but after the bug summary and the bug url.

> Source/WebCore/html/HTMLPlugInElement.cpp:83
>          _NPN_ReleaseObject(m_NPObject);

Can this trigger a layout or style recalc? i.e. is the np object gets notified synchronously here? If so, it's not safe to do this.

> Source/WebCore/testing/Internals.idl:125
> +        unsigned long numberOfLiveNodes();
> +        unsigned long numberOfLiveDocuments();

It seems like we should just wrap this inside ENABLE(INSPECTOR).
It's misleading for it to return 0. Note that the right way to do that is appending [Conditional=INSPECTOR] on each function declaration.

> LayoutTests/plugins/netscape-dom-access-and-reload.html:10
> +    // Make the plugin retrieve a DOM element to itself. This exercises
> +    // https://bugs.webkit.org/show_bug.cgi?id=80428
> +    // (HTMLPluginElement is not destroyed on reload or navigation if
> +    // getNPObject is called)

This comment is redundant since it's included in the body.

> LayoutTests/plugins/netscape-dom-access-and-reload.html:13
> +    // Reload the page once.

This comment repeats the code. Please remove.

> LayoutTests/plugins/netscape-dom-access-and-reload.html:28
> +        window.location.reload();

No need for "window.".

> LayoutTests/plugins/netscape-dom-access-and-reload.html:35
> +        var numberOfLiveDocuments = window.internals.numberOfLiveDocuments();

No need for "window.".

> LayoutTests/plugins/netscape-dom-access-and-reload.html:41
> +                "FAILED; numberOfLiveDocuments returned 0. This test is only "
> +                "valid if the Inspector is enabled.";

If we had made numberOfLiveDocuments conditional, you can detect this in the script.

[Dave Michael]

Created attachment 133193 [details]
Patch

[Dave Michael]

Comment on attachment 133170 [details]
Patch

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

>> Source/WebCore/html/HTMLPlugInElement.cpp:83
>>          _NPN_ReleaseObject(m_NPObject);
> 
> Can this trigger a layout or style recalc? i.e. is the np object gets notified synchronously here? If so, it's not safe to do this.

I'm not certain if it's possible for the plugin to cause a layout from its NPP_Deallocate function (it may be prohibited). But it's scary enough that I used a Timer to defer the release.

(Good catch)

>> Source/WebCore/testing/Internals.idl:125
>> +        unsigned long numberOfLiveDocuments();
> 
> It seems like we should just wrap this inside ENABLE(INSPECTOR).
> It's misleading for it to return 0. Note that the right way to do that is appending [Conditional=INSPECTOR] on each function declaration.

Good idea. Done.

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:10
>> +    // getNPObject is called)
> 
> This comment is redundant since it's included in the body.

Removed.

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:13
>> +    // Reload the page once.
> 
> This comment repeats the code. Please remove.

I removed it. I guess I thought it was helpful since it summarizes the purpose of the next 7 lines of code.

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:28
>> +        window.location.reload();
> 
> No need for "window.".

removed.

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:35
>> +        var numberOfLiveDocuments = window.internals.numberOfLiveDocuments();
> 
> No need for "window.".

removed.

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:41
>> +                "valid if the Inspector is enabled.";
> 
> If we had made numberOfLiveDocuments conditional, you can detect this in the script.

Done; this is cleaner!

[Dimitri Glazkov (Google)]

Comment on attachment 133193 [details]
Patch

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

> Source/WebCore/html/HTMLPlugInElement.cpp:86
> +    m_releaseNPObjectTimer.startOneShot(0);

Won't queuePostAttachCallback work instead? Seems like it was designed for this purpose.

> LayoutTests/plugins/netscape-dom-access-and-reload.html:38
> +              document.getElementById("result").innerHTML = "FAILED; This test is only valid if the Inspector is enabled.";

Inspector _and_ DumpRenderTree, right?

[Dave Michael]

Created attachment 133269 [details]
Patch

[Dave Michael]

Comment on attachment 133193 [details]
Patch

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

Thanks for looking! I uploaded a new patch.

>> Source/WebCore/html/HTMLPlugInElement.cpp:86
>> +    m_releaseNPObjectTimer.startOneShot(0);
> 
> Won't queuePostAttachCallback work instead? Seems like it was designed for this purpose.

Perhaps... that sounds like it's meant for stuff to do just after attach, rather than detach? But in any case, I just didn't know about it :-)

Turns out, though, that I was just not thinking straight when going over rniwa's comments. Releasing HTMLPlugInElement doesn't give the plugin an opportunity to do anything. So I changed it back to just releasing immediately, and I added an explanation to the ChangeLog (as suggested by rniwa on IRC).

>> LayoutTests/plugins/netscape-dom-access-and-reload.html:38
>> +              document.getElementById("result").innerHTML = "FAILED; This test is only valid if the Inspector is enabled.";
> 
> Inspector _and_ DumpRenderTree, right?

Done

[Dimitri Glazkov (Google)]

Comment on attachment 133269 [details]
Patch

Looks good. I would like Dr. Seidel to give it one more look.

[Eric Seidel (no email)]

Comment on attachment 133269 [details]
Patch

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

LGTM.

> Source/WebCore/html/HTMLPlugInElement.h:30
> +#include "Timer.h"

Why is this needed?

[Dave Michael]

Created attachment 133307 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 133307 [details]
Patch

Fantastic!

[WebKit Review Bot]

Comment on attachment 133307 [details]
Patch

Clearing flags on attachment: 133307

Committed r111754: <http://trac.webkit.org/changeset/111754>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to comment #41)
> (From update of attachment 133307 [details])
> Clearing flags on attachment: 133307
> 
> Committed r111754: <http://trac.webkit.org/changeset/111754>

The test added in this change has been failing in Mac WebKit2 ever since. Filed bug 82020.

[Ryosuke Niwa]

I think this patch caused plugins/reloadplugins-and-pages.html to fail on all platforms:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=plugins%2Freloadplugins-and-pages.html

[Ryosuke Niwa]

Looking at http://trac.webkit.org/log/?verbose=on&rev=111757&stop_rev=111749, I'm certain this patch caused the failure.

Given that the test added by this patch is failing on Mac WebKit 2 and it caused a regression, I'm going to rollout the patch along with mitz's WK2 rebaseline in the next half an hour or so. Please let me know ASAP if you know how to fix it such that we don't have to roll out the patch.

[Ryosuke Niwa]

Reopen the bug since the patch was rolled out in http://trac.webkit.org/changeset/111839.

[Ryosuke Niwa]

http://crbug.com/114023/

[Dave Michael]

Created attachment 133506 [details]
Patch

[Dave Michael]

Created attachment 133507 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 133507 [details]
Patch

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

> LayoutTests/ChangeLog:11
> +        Due to unfortunate copy/paste laziness, the new test was using the same
> +        window.sessionStorage as plugins/reloadplugins-and-pages.html, so that
> +        if the tests were run in the same session, reloadplugins-and-pages.html
> +        would *not* reload as it was supposed to, causing a text mismatch. This
> +        patch uses a more appropriate and unique name so that these two tests
> +        won't affect each other.

Isn't this a DRT bug? It seems like we should be re-setting the session storage whenever new test is loaded.

e.g. Mac port has:
http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/mac/DumpRenderTree.mm#L1225

> LayoutTests/ChangeLog:13
> +        Reviewed by NOBODY (OOPS!).

This line should appear before the description.

[Dave Michael]

Created attachment 133512 [details]
Patch

[Dave Michael]

Comment on attachment 133507 [details]
Patch

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

>> LayoutTests/ChangeLog:11
>> +        won't affect each other.
> 
> Isn't this a DRT bug? It seems like we should be re-setting the session storage whenever new test is loaded.
> 
> e.g. Mac port has:
> http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/mac/DumpRenderTree.mm#L1225

I wouldn't know. I filed a bug: https://bugs.webkit.org/show_bug.cgi?id=82068

Hopefully we can deal with that separately?

>> LayoutTests/ChangeLog:13
>> +        Reviewed by NOBODY (OOPS!).
> 
> This line should appear before the description.

Augh, someday I'll get the ChangeLog stuff right!

Thanks, Done :-)

[Ryosuke Niwa]

Comment on attachment 133512 [details]
Patch

Forwarding eric's r+. (You should modify Reviewed by line to include both names of our names if you're so inclined to include my name).

[Dave Michael]

Created attachment 133528 [details]
Patch

[WebKit Review Bot]

Comment on attachment 133528 [details]
Patch

Clearing flags on attachment: 133528

Committed r111890: <http://trac.webkit.org/changeset/111890>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to comment #55)
> (From update of attachment 133528 [details])
> Clearing flags on attachment: 133528
> 
> Committed r111890: <http://trac.webkit.org/changeset/111890>

The test added in this change has been failing in Mac WebKit2 ever since. This is the second time a change for this bug does this. Please keep an eye on build.webkit.org after committing a change to see if any new tests are failing.