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.
Created test case that triggers this failure. Still investigating a fix.
Created attachment 77377 [details] patch
Attachment 77377 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7205142
Attachment 77377 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7313148
Comment on attachment 77377 [details] patch r- This patch breaks several builds (and may have style issues as well).
Created attachment 77913 [details] updated patch Fixed include ordering and restored initPlugins() to avoid breaking platform builds.
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)?
(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.
Created attachment 78179 [details] updated patch
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.
Created attachment 78187 [details] updated patch netscape text removed. observer -> destructionObserver in method name and member var.
Comment on attachment 78187 [details] updated patch r=me
Comment on attachment 78187 [details] updated patch Clearing flags on attachment: 78187 Committed r75217: <http://trac.webkit.org/changeset/75217>
All reviewed patches have been landed. Closing bug.
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
http://trac.webkit.org/changeset/75217 might have broken GTK Linux 32-bit Debug
The failure on Chrome Windows is tracked by bug 52062