PluginDocumentParser::pluginWidgetFromDocument assumes that the "embed" element is the first child of the body. However, for Chromium, an extension can modify the DOM to add other elements before the "embed" element. The method should search for the "embed" element rather than use the first child. Working on a fix for this. This causes the Chromium bug (http://crbug.com/51342).
Created attachment 69719 [details] Patch for the fix
Comment on attachment 69719 [details] Patch for the fix View in context: https://bugs.webkit.org/attachment.cgi?id=69719&action=review > WebCore/html/PluginDocument.cpp:-87 > -#if ENABLE(OFFLINE_WEB_APPLICATIONS) oops
Created attachment 69726 [details] Updated patch with unrelated changes removed.
Created attachment 69727 [details] Fixed spaces.
Comment on attachment 69727 [details] Fixed spaces. Clearing flags on attachment: 69727 Committed r69097: <http://trac.webkit.org/changeset/69097>
All reviewed patches have been landed. Closing bug.
There is a discussion of this change on webkit-changes mailing list.
Created attachment 69856 [details] New implementation that relies on PluginDocument remembering the plugin node.
Uploaded new implementation as per discussion on webkit-changes.
Comment on attachment 69856 [details] New implementation that relies on PluginDocument remembering the plugin node. This does seem much better. Thanks!
Comment on attachment 69856 [details] New implementation that relies on PluginDocument remembering the plugin node. View in context: https://bugs.webkit.org/attachment.cgi?id=69856&action=review > WebCore/html/PluginDocument.h:60 > + RefPtr<Node> m_pluginNode; Is it okay for a Document to hold a reference to a node like this? Won’t it create a ref cycle and cause a leak?
Comment on attachment 69856 [details] New implementation that relies on PluginDocument remembering the plugin node. Clearing flags on attachment: 69856 Committed r69159: <http://trac.webkit.org/changeset/69159>
Comment on attachment 69856 [details] New implementation that relies on PluginDocument remembering the plugin node. View in context: https://bugs.webkit.org/attachment.cgi?id=69856&action=review >> WebCore/html/PluginDocument.h:60 >> + RefPtr<Node> m_pluginNode; > > Is it okay for a Document to hold a reference to a node like this? Won’t it create a ref cycle and cause a leak? Yeah I was concerned about that too. But I was also afraid of holding on to a pointer that was no longer valid. I did some debugging and found that it does indeed cause a ref cycle. I can change it to Node* but I am not sure when to clear this variable. Any suggestions?
(In reply to comment #14) > (From update of attachment 69856 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=69856&action=review > > >> WebCore/html/PluginDocument.h:60 > >> + RefPtr<Node> m_pluginNode; > > > > Is it okay for a Document to hold a reference to a node like this? Won’t it create a ref cycle and cause a leak? > > Yeah I was concerned about that too. But I was also afraid of holding on to a pointer that was no longer valid. I did some debugging and found that it does indeed cause a ref cycle. I can change it to Node* but I am not sure when to clear this variable. Any suggestions? I asked and was told that this does not cause a leak.
Comment on attachment 69856 [details] New implementation that relies on PluginDocument remembering the plugin node. View in context: https://bugs.webkit.org/attachment.cgi?id=69856&action=review >>>> WebCore/html/PluginDocument.h:60 >>>> + RefPtr<Node> m_pluginNode; >>> >>> Is it okay for a Document to hold a reference to a node like this? Won’t it create a ref cycle and cause a leak? >> >> Yeah I was concerned about that too. But I was also afraid of holding on to a pointer that was no longer valid. I did some debugging and found that it does indeed cause a ref cycle. I can change it to Node* but I am not sure when to clear this variable. Any suggestions? > > I asked and was told that this does not cause a leak. Hmm, I did some debugging on Windows Chromium and it did seem to cause a leak. Maybe I missed something. I will debug some more and update this bug. Thanks for the help.
Created attachment 70014 [details] Fixed memory leak caused by circular ref count
Last patch had a memory leak. Fixed that.
Comment on attachment 70014 [details] Fixed memory leak caused by circular ref count R=me, thanks Sanjeev!
Comment on attachment 70014 [details] Fixed memory leak caused by circular ref count Clearing flags on attachment: 70014 Committed r69268: <http://trac.webkit.org/changeset/69268>