HTMLPlugInElement persists until page teardown if the plugin requests the script object for it

• Component: Plug-ins
• Status: RESOLVED FIXED

Any plugin which requests the script object for the containing element causes the HTMLPlugInElement to generate an NPObject wrapper for itself, which holds a reference to it.  It returns this to the plugin, but also retains a reference internally, to return if the plugin requests the script object again.  This creates a cyclical reference which prevents the HTMLPlugInElement from being torn down.

To reproduce the issue:
1.  Arrange to detect HTMLPlugInElement leaks (e.g. and printout to constructor & destructor).
2.  Verify that you see HTMLPlugInElement torn-down, by running the TestNetscapePlugIn "getURL" test, for example.
3.  Run the TestNetscapePlugIn "testDOMAccess" test, which causes the plugin to request an NPObject for the plugin element, and to then release it when the plugin is torn down.
4.  If you're feeling eager, run that test several times from within the same page, creating and tearing down TestNetscapePlugIn elements, so that multiple elements will be leaked.

Actual Results:
The HTMLPlugInElement should be torn down correctly in (2), but not be in (3) and (4).

Expected Results:
The HTMLPlugInElement should be torn down when the plug-in is destroyed.

Builds & Platforms:
This has been observed running WebKit within the Chromium browser, on several platforms.

Created attachment 103885 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 103885 [details]
Patch

I'm hugely suspicious of this patch. Before changing cross-platform code, please at least confirm that this problem affects other ports.

I've reproduced the issue with Linux/GtkLauncher.

It won't reproduce with Mac/Safari, because the relevant code-path is never invoked.

To clarify, the leak occurs only while the calling page is active; when the page is destroyed the objects are still correctly torn down.

[Alexey Proskuryakov]

> It won't reproduce with Mac/Safari, because the relevant code-path is never invoked.

Are you saying that HTMLPlugInElement::removedFromDocument() is never called on Mac?

No, HTMLPlugInElement::getNPObject() never seems to be called under Safari.

[Alexey Proskuryakov]

OK, so you are changing a function that is called in Safari. Why is it ok, provided that the issue doesn't exist in the Mac port?

More generally, what is there difference between ports?

Sorry for the confusion!

The Chromium and Linux/Gtk ports call HTMLPlugInElement::getNPObject() to fetch the plugin element's script object when the plugin requests it.  At that point HTMLPlugInElement caches the object, retaining a reference to it.  I think Mac/Safari must have its own method of generating a script object for the HTMLPlugInElement, without calling getNPObject().

The plugin element script object is cached presumably so that the plugin will just increase the reference count on a single object if it requests it repeatedly, rather than generating distinct objects.  It's therefore reasonable to clear the cached reference when the plugin has been torn down, which should be the case when removedFromDocument() is notified.

Clearing the reference in detach() also sounds plausible, but since detach() is notified when an element is hidden, I'm not sure whether that would be correct?

The release of the reference in removedFromDocument() is only done if a reference is cached, which will only be the case if getNPObject() was previously called to generate the script object.  This also makes it safe on platforms which never call getNPObject(), since on those platforms there will never be a cached reference to release.

[Alexey Proskuryakov]

From code inspection, I see that WebKit1 is definitely calling getNPObject(), it's in WebNetscapePluginView.mm. This code path is for in-process plug-ins, so it should be hit when Safari runs in 32-bit WebKit1 mode.

Even though this mode is old, I'd be somewhat surprised if we leaked there.

(In reply to comment #8)
> From code inspection, I see that WebKit1 is definitely calling getNPObject(), it's in WebNetscapePluginView.mm. This code path is for in-process plug-ins, so it should be hit when Safari runs in 32-bit WebKit1 mode.

That would certainly explain it.

> Even though this mode is old, I'd be somewhat surprised if we leaked there.

It'll only manifest for plugins that fetch the script object for their element, and the memory will be freed when the page is torn down, so it's a leak you'd only notice if you were adding & removing plugins on a page for quite a while.

(In reply to comment #9)
> (In reply to comment #8)
> > From code inspection, I see that WebKit1 is definitely calling getNPObject(), it's in WebNetscapePluginView.mm. This code path is for in-process plug-ins, so it should be hit when Safari runs in 32-bit WebKit1 mode.
> 
> That would certainly explain it.
> 
> > Even though this mode is old, I'd be somewhat surprised if we leaked there.
> 
> It'll only manifest for plugins that fetch the script object for their element, and the memory will be freed when the page is torn down, so it's a leak you'd only notice if you were adding & removing plugins on a page for quite a while.

I've tried running Safari 32-bit (via the option under Get Info), but I'm still not seeing getNPObject getting called.

(In reply to comment #10)
> I've tried running Safari 32-bit (via the option under Get Info), but I'm still not seeing getNPObject getting called.

I've determined that I was still running 64-bit; I've now got Safari running 32-bit, but of course I can't load my WebKit build nor plugin into it.

Is there some flag to provide to build-webkit to get a 32-bit or fat build - I can't find any documentation on how to do that?

[Alexey Proskuryakov]

Yes, set-webkit-configuration --32-bit.

You will also want to create a WebKit1 window:
1. In Terminal: defaults write com.apple.Safari IncludeInternalDebugMenu 1
2. Restart Safari.
3. Cmd+Option+N (there will be [SP] in window title, standing for "single process").

(In reply to comment #12)
> Yes, set-webkit-configuration --32-bit.
> 
> You will also want to create a WebKit1 window:
> 1. In Terminal: defaults write com.apple.Safari IncludeInternalDebugMenu 1
> 2. Restart Safari.
> 3. Cmd+Option+N (there will be [SP] in window title, standing for "single process").

Thanks - will give that a shot!

(In reply to comment #13)
> (In reply to comment #12)
> > Yes, set-webkit-configuration --32-bit.
> > 
> > You will also want to create a WebKit1 window:
> > 1. In Terminal: defaults write com.apple.Safari IncludeInternalDebugMenu 1
> > 2. Restart Safari.
> > 3. Cmd+Option+N (there will be [SP] in window title, standing for "single process").
> 
> Thanks - will give that a shot!

I've got 32-bit Safari running, and a Single-Process window opening, but it segfaults running the TestNetscapePlugIn test, in different ways depending on whether I run it with the system WebKit framework or my debug build.

(In reply to comment #14)
> I've got 32-bit Safari running, and a Single-Process window opening, but it segfaults running the TestNetscapePlugIn test, in different ways depending on whether I run it with the system WebKit framework or my debug build.

Since the test crashes Safari 32-bit Single-Process, I've instead verified that:

1.  Running single-process, the plugin does cause HTMLPlugInElement::getNPObject() to be invoked, before Safari crashes.

2.  Running multi-process, with HTMLPlugInElement modified to call getNPObject itself in getInstance(), the HTMLPlugInElements are not released until the page is closed.

3.  Running multi-process, with the modification in (2), and my patch applied, the elements are torn down as they are removed from the document, as you'd expect.

These tests were run with removedFromDocument() modified to invoke gcController().garbageCollectNow(), so as to force timely collection of the element reference.

[Alexey Proskuryakov]

Thank you! I guess cross-platform code is the right place for the fix then.

Are plug-ins completely torn down when an element is removed from document (and then re-created from scratch if the element is put back)?

+        Release the reference held by HTMLPlugInElement to the wrapping NPObject it may have created for itself, when the element is removed from the document.  This breaks a cyclical reference that would otherwise cause the element to be leaked.

Calling this a leak is misleading given that the memory is reclaimed when document is closed.

CC'ing Anders, who would likely be the best reviewer for this patch anyway.

(In reply to comment #16)
> Thank you! I guess cross-platform code is the right place for the fix then.
> 
> Are plug-ins completely torn down when an element is removed from document (and then re-created from scratch if the element is put back)?

My understanding is that plugin lifetimes are tied to the relevant render widgets; if the element is removed or hidden then the plugin will be destroyed.  If that is correct then the script object could be cleared in detach() instead, but I chose to be conservative since I couldn't find any documentation of the intended behaviour.  Things behave as described above under Chromium, but I've not checked other browsers.

> +        Release the reference held by HTMLPlugInElement to the wrapping NPObject it may have created for itself, when the element is removed from the document.  This breaks a cyclical reference that would otherwise cause the element to be leaked.
> 
> Calling this a leak is misleading given that the memory is reclaimed when document is closed.

Yes, although from the point of view of a long-lived document, the two are indistinguishable.  I'll try to come up with more precise wording, though.

> CC'ing Anders, who would likely be the best reviewer for this patch anyway.

Thanks!

Created attachment 105407 [details]
Patch

(In reply to comment #18)
> Created an attachment (id=105407) [details]
> Patch

Any progress on this?

(In reply to comment #19)
> (In reply to comment #18)
> > Created an attachment (id=105407) [details] [details]
> > Patch
> 
> Any progress on this?

Ping! :)

[Anders Carlsson]

Comment on attachment 105407 [details]
Patch

Looks fine, r+. Sorry about the delay!

[WebKit Review Bot]

Comment on attachment 105407 [details]
Patch

Clearing flags on attachment: 105407

Committed r94831: <http://trac.webkit.org/changeset/94831>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.