Bug 45770

Summary:

When a live frame is moved between pages, some plug-in DOM methods cease to function

Product:

WebKit

Reporter:

Alexey Proskuryakov <ap>

Component:

Frames

Assignee:

Jenn Braithwaite <jennb>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, aestes, commit-queue, dglazkov, dimich, eric, gustavo, jennb, senorblanco, simon.fraser, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
patch	levin: review-
updated patch	dimich: review-
updated patch	none
updated patch	none

Alexey Proskuryakov

Reported 2010-09-14 12:28:33 PDT

There is a per-Page PluginData object which Plugin and MimeType reference. If a document moves to another pages, these references should all be updated. On my test cases, this results in calls like navigator.plugins[0].item(0).enabledPlugin return null, but if we missed a null check somewhere, it can also crash once the other page is destroyed.

Attachments
patch (18.26 KB, patch) 2010-12-23 15:38 PST, Jenn Braithwaite	levin: review-	Details Formatted Diff Diff
updated patch (17.96 KB, patch) 2011-01-04 11:35 PST, Jenn Braithwaite	dimich: review-	Details Formatted Diff Diff
updated patch (21.08 KB, patch) 2011-01-06 15:30 PST, Jenn Braithwaite	no flags	Details Formatted Diff Diff
updated patch (21.21 KB, patch) 2011-01-06 16:32 PST, Jenn Braithwaite	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Jenn Braithwaite

Comment 1 2010-12-22 15:37:21 PST

Created test case that triggers this failure. Still investigating a fix.

Jenn Braithwaite

Comment 2 2010-12-23 15:38:48 PST

Created attachment 77377 [details] patch

WebKit Review Bot

Comment 3 2010-12-23 15:58:45 PST

Attachment 77377 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7205142

WebKit Review Bot

Comment 4 2010-12-23 16:11:30 PST

Attachment 77377 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7313148

David Levin

Comment 5 2010-12-27 11:17:29 PST

Comment on attachment 77377 [details] patch r- This patch breaks several builds (and may have style issues as well).

Jenn Braithwaite

Comment 6 2011-01-04 11:35:47 PST

Created attachment 77913 [details] updated patch Fixed include ordering and restored initPlugins() to avoid breaking platform builds.

Dmitry Titov

Comment 7 2011-01-06 12:06:55 PST

Comment on attachment 77913 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=77913&action=review Few comments (r- to add description to the test) > LayoutTests/fast/frames/iframe-reparenting-plugins.html:55 > +<body onload="test()"> It would be nice to have a short text description of the test here - what it does, what constitutes the failure and success. Also, why we always expect to find a plugin there. > WebCore/page/Frame.h:72 > + class FrameObserver { If this is only used to get a callback on frame destruction, it could be more specific and be called FrameDestructionObserver, with frameDestroyed() as a name of its callback function. So that it doesn't get an assortment of various callbacks in the future. > WebCore/plugins/DOMMimeType.cpp:71 > + if (!m_frame || !m_frame->loader()->subframeLoader()->allowPlugins(NotAboutToInstantiatePlugin)) How important is the change in behavior here? Lets say I have a page on apple.com embedding an iframe from YouTube. Could it be possible that Flash is disabled for apple.com but not YouTube, and while old behavior would disable Flash in that combination (because it could use settings for apple.com), the new one would not. > WebCore/plugins/PluginData.cpp:38 > + m_page = 0; // Page only needed by initPlugins if USE(PLATFORM_STRATEGIES). Is it possible to only have m_page if USE(PLATFORM_STRATEGIES)?

Jenn Braithwaite

Comment 8 2011-01-06 13:07:26 PST

(In reply to comment #7) > (From update of attachment 77913 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=77913&action=review > > Few comments (r- to add description to the test) > > > LayoutTests/fast/frames/iframe-reparenting-plugins.html:55 > > +<body onload="test()"> > > It would be nice to have a short text description of the test here - what it does, what constitutes the failure and success. > Also, why we always expect to find a plugin there. Will change. > > > WebCore/page/Frame.h:72 > > + class FrameObserver { > > If this is only used to get a callback on frame destruction, it could be more specific and be called FrameDestructionObserver, with frameDestroyed() as a name of its callback function. So that it doesn't get an assortment of various callbacks in the future. Will change. > > > WebCore/plugins/DOMMimeType.cpp:71 > > + if (!m_frame || !m_frame->loader()->subframeLoader()->allowPlugins(NotAboutToInstantiatePlugin)) > > How important is the change in behavior here? Lets say I have a page on apple.com embedding an iframe from YouTube. Could it be possible that Flash is disabled for apple.com but not YouTube, and while old behavior would disable Flash in that combination (because it could use settings for apple.com), the new one would not. I don't know the answer to this one. However, I can make it use m_frame->page()->mainFrame() instead to keep the old behavior. > > > WebCore/plugins/PluginData.cpp:38 > > + m_page = 0; // Page only needed by initPlugins if USE(PLATFORM_STRATEGIES). > > Is it possible to only have m_page if USE(PLATFORM_STRATEGIES)? I can do that.

Jenn Braithwaite

Comment 9 2011-01-06 15:30:36 PST

Created attachment 78179 [details] updated patch

Dmitry Titov

Comment 10 2011-01-06 16:24:15 PST

Comment on attachment 78179 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=78179&action=review Great! A couple of nits: > LayoutTests/fast/frames/resources/iframe-reparenting-plugins-iframe-content.html:11 > + return "FAIL: Failed to find netscape test plugin\n"; could remove 'netscape'. > WebCore/page/Frame.h:93 > + void addObserver(FrameDestructionObserver*); addObserver -> addDestructionObserver ? same for removeObserver and m_observers below.

Jenn Braithwaite

Comment 11 2011-01-06 16:32:13 PST

Created attachment 78187 [details] updated patch netscape text removed. observer -> destructionObserver in method name and member var.

Dmitry Titov

Comment 12 2011-01-06 16:33:11 PST

Comment on attachment 78187 [details] updated patch r=me

WebKit Commit Bot

Comment 13 2011-01-06 17:59:05 PST

Comment on attachment 78187 [details] updated patch Clearing flags on attachment: 78187 Committed r75217: <http://trac.webkit.org/changeset/75217>

WebKit Commit Bot

Comment 14 2011-01-06 17:59:10 PST

All reviewed patches have been landed. Closing bug.

Stephen White

Comment 15 2011-01-06 19:53:14 PST

This new test seems to be breaking on the Windows canary: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&master=ChromiumWebkit&tests=fast%2Fframes%2Fiframe-reparenting-plugins.html

WebKit Review Bot

Comment 16 2011-01-07 02:28:54 PST

http://trac.webkit.org/changeset/75217 might have broken GTK Linux 32-bit Debug

Dmitry Titov

Comment 17 2011-01-07 16:45:22 PST

The failure on Chrome Windows is tracked by bug 52062

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