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.
Created attachment 130409 [details] Patch
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.
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?
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 > +2012-03-06 Dave Michael <dmichael@chromium.org> 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....
Created attachment 132057 [details] Patch
(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 > > +2012-03-06 Dave Michael <dmichael@chromium.org> > > 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....
Comment on attachment 132057 [details] Patch Thanks for the review. I updated the changelog and tried to explain why there's no test.
(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.
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+.
Created attachment 132963 [details] Patch
Comment on attachment 132963 [details] Patch Attachment 132963 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12072144
Comment on attachment 132963 [details] Patch Attachment 132963 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12066170
Created attachment 132967 [details] Patch
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?
Comment on attachment 132967 [details] Patch Attachment 132967 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12066180
Comment on attachment 132967 [details] Patch Attachment 132967 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12072159
Comment on attachment 132967 [details] Patch Attachment 132967 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12066204
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?
Created attachment 133088 [details] Patch
Comment on attachment 133088 [details] Patch Attachment 133088 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12071570
Comment on attachment 133088 [details] Patch Attachment 133088 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12090539
Comment on attachment 133088 [details] Patch Attachment 133088 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12072595
Created attachment 133118 [details] Patch
+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)
Comment on attachment 133118 [details] Patch Attachment 133118 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12066633
Created attachment 133132 [details] Patch
./.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
Comment on attachment 133132 [details] Patch Attachment 133132 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12072631
Created attachment 133170 [details] Patch
Comment on attachment 133170 [details] Patch I think I have all the right incantations to make this build on all the platforms.
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.
Created attachment 133193 [details] Patch
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!
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?
Created attachment 133269 [details] Patch
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
Comment on attachment 133269 [details] Patch Looks good. I would like Dr. Seidel to give it one more look.
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?
Created attachment 133307 [details] Patch
Comment on attachment 133307 [details] Patch Fantastic!
Comment on attachment 133307 [details] Patch Clearing flags on attachment: 133307 Committed r111754: <http://trac.webkit.org/changeset/111754>
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.
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
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.
Reopen the bug since the patch was rolled out in http://trac.webkit.org/changeset/111839.
http://crbug.com/114023/
Created attachment 133506 [details] Patch
Created attachment 133507 [details] Patch
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.
Created attachment 133512 [details] Patch
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 :-)
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).
Created attachment 133528 [details] Patch
Comment on attachment 133528 [details] Patch Clearing flags on attachment: 133528 Committed r111890: <http://trac.webkit.org/changeset/111890>
(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.