Bug 47129 – PluginDocumentParser::pluginWidgetFromDocument should look for "embed" element rather than just use the first child

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

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

[Darin Fisher (:fishd, Google)]

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.

[WebKit Commit Bot]

Comment on attachment 69727 [details]
Fixed spaces.

Clearing flags on attachment: 69727

Committed r69097: <http://trac.webkit.org/changeset/69097>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

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.

[Darin Fisher (:fishd, Google)]

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?

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

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.

[Darin Fisher (:fishd, Google)]

Comment on attachment 70014 [details]
Fixed memory leak caused by circular ref count

R=me, thanks Sanjeev!

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.