Bug 47129

Summary:

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

Product:

WebKit

Reporter:

sanjeevr

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aestes, commit-queue, fishd, mitz

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
Patch for the fix	fishd: review-, fishd: commit-queue-
Updated patch with unrelated changes removed.	none
Fixed spaces.	none
New implementation that relies on PluginDocument remembering the plugin node.	none
Fixed memory leak caused by circular ref count	none

sanjeevr

Reported 2010-10-04 16:04:25 PDT

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).

Attachments
Patch for the fix (2.19 KB, patch) 2010-10-04 16:44 PDT, sanjeevr	fishd: review- fishd: commit-queue-	Details Formatted Diff Diff
Updated patch with unrelated changes removed. (1.63 KB, patch) 2010-10-04 17:28 PDT, sanjeevr	no flags	Details Formatted Diff Diff
Fixed spaces. (1.59 KB, patch) 2010-10-04 18:30 PDT, sanjeevr	no flags	Details Formatted Diff Diff
New implementation that relies on PluginDocument remembering the plugin node. (3.70 KB, patch) 2010-10-05 15:51 PDT, sanjeevr	no flags	Details Formatted Diff Diff
Fixed memory leak caused by circular ref count (1.65 KB, patch) 2010-10-06 17:03 PDT, sanjeevr	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

sanjeevr

Comment 1 2010-10-04 16:44:35 PDT

Created attachment 69719 [details] Patch for the fix

Darin Fisher (:fishd, Google)

Comment 2 2010-10-04 17:04:09 PDT

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

sanjeevr

Comment 3 2010-10-04 17:28:46 PDT

Created attachment 69726 [details] Updated patch with unrelated changes removed.

sanjeevr

Comment 4 2010-10-04 18:30:58 PDT

Created attachment 69727 [details] Fixed spaces.

WebKit Commit Bot

Comment 5 2010-10-05 01:43:00 PDT

Comment on attachment 69727 [details] Fixed spaces. Clearing flags on attachment: 69727 Committed r69097: <http://trac.webkit.org/changeset/69097>

WebKit Commit Bot

Comment 6 2010-10-05 01:43:05 PDT

All reviewed patches have been landed. Closing bug.

Alexey Proskuryakov

Comment 7 2010-10-05 11:42:29 PDT

There is a discussion of this change on webkit-changes mailing list.

sanjeevr

Comment 8 2010-10-05 15:51:32 PDT

Created attachment 69856 [details] New implementation that relies on PluginDocument remembering the plugin node.

sanjeevr

Comment 9 2010-10-05 15:55:05 PDT

Uploaded new implementation as per discussion on webkit-changes.

Darin Fisher (:fishd, Google)

Comment 10 2010-10-05 15:59:57 PDT

Comment on attachment 69856 [details] New implementation that relies on PluginDocument remembering the plugin node. This does seem much better. Thanks!

mitz

Comment 11 2010-10-05 16:07:26 PDT

WebKit Commit Bot

Comment 12 2010-10-05 16:15:21 PDT

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

Comment 13 2010-10-05 16:15:27 PDT

All reviewed patches have been landed. Closing bug.

sanjeevr

Comment 14 2010-10-05 17:33:05 PDT

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?

mitz

Comment 15 2010-10-05 17:36:47 PDT

(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.

sanjeevr

Comment 16 2010-10-05 17:54:46 PDT

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.

sanjeevr

Comment 17 2010-10-06 17:03:24 PDT

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

sanjeevr

Comment 18 2010-10-06 17:03:45 PDT

Last patch had a memory leak. Fixed that.

Darin Fisher (:fishd, Google)

Comment 19 2010-10-06 17:06:46 PDT

Comment on attachment 70014 [details] Fixed memory leak caused by circular ref count R=me, thanks Sanjeev!

WebKit Commit Bot

Comment 20 2010-10-06 18:43:10 PDT

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

Comment 21 2010-10-06 18:43:16 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.