Bug 46773 – REGRESSION (r66129): Loading full-frame .swf file crashes with flash blocker extension enabled

• Component: Plug-ins
• Status: RESOLVED FIXED

[Brian Weinstein]

REGRESSION (r66129): Loading full-frame .swf file crashes with flash blocker extension enabled.

r66129 started injecting start scripts into plugin documents, which allowed the loading of plugin documents to be blocked, but this could lead to a crash.

[Brian Weinstein]

<rdar://problem/8390975>

[Brian Weinstein]

Created attachment 69152 [details]
[PATCH] Fix

[Adam Roben (:aroben)]

Comment on attachment 69152 [details]
[PATCH] Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=69152&action=review

> WebCore/ChangeLog:17
> +        No new tests because it is not possible to cancel a plugin document load using
> +        beforeload without extensions.

But we can test user scripts. So can't we test this?

[Brian Weinstein]

(In reply to comment #3)
> (From update of attachment 69152 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69152&action=review
> 
> > WebCore/ChangeLog:17
> > +        No new tests because it is not possible to cancel a plugin document load using
> > +        beforeload without extensions.
> 
> But we can test user scripts. So can't we test this?

No, we can't test this. When we create a Plugin document in an iframe, and we try to cancel the load of the Plugin from the iframes owner document, that prevents the Plugin document from being created, which is different behavior than what an extension will give you when it cancels the load of the Plugin inside the Plugin document. I will double check to make sure, however.

[Brian Weinstein]

Created attachment 69231 [details]
[PATCH] Fix with tests

[Darin Adler]

Comment on attachment 69231 [details]
[PATCH] Fix with tests

View in context: https://bugs.webkit.org/attachment.cgi?id=69231&action=review

> WebCore/html/HTMLEmbedElement.cpp:168
> +        if (document()->isPluginDocument()) {
> +            document()->frame()->loader()->activeDocumentLoader()->mainResourceLoader()->cancel();
> +            toPluginDocument(document())->setPluginLoadWasCancelled(true);
> +        }

This code needs a why comment.

It’s not great factoring to have this code here; can we put the code in a better place? For example, a virtual function on Document? A document loader function? A plug-in document function that combines both the loader cancel and setting the flag? I don’t have a specific idea.

> WebCore/html/PluginDocument.cpp:132
> +        }

I think the local variable should just be named “widget”. Is there another widget other than then plug-in widget?

The definition of the local variable could be inside the if statement.

It’s not clear why setShouldBufferData(false) needs to be inside the if statement.

> WebCore/html/PluginDocument.h:47
> +    bool pluginLoadWasCancelled() const { return m_pluginLoadWasCancelled; }
> +    void setPluginLoadWasCancelled(bool wasCancelled) { m_pluginLoadWasCancelled = wasCancelled; }

Does this really need to be specific to plug-ins? I don’t see how the fact that a load was cancelled is a plug-in specific concept. Can this boolean be on DocumentLoader instead?

[Brian Weinstein]

(In reply to comment #6)
> (From update of attachment 69231 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69231&action=review
> 
> > WebCore/html/HTMLEmbedElement.cpp:168
> > +        if (document()->isPluginDocument()) {
> > +            document()->frame()->loader()->activeDocumentLoader()->mainResourceLoader()->cancel();
> > +            toPluginDocument(document())->setPluginLoadWasCancelled(true);
> > +        }
> 
> This code needs a why comment.

Added.

> 
> It’s not great factoring to have this code here; can we put the code in a better place? For example, a virtual function on Document? A document loader function? A plug-in document function that combines both the loader cancel and setting the flag? I don’t have a specific idea.

I like the third idea. Something like PluginDocument::cancelManualPluginLoad. 

> 
> > WebCore/html/PluginDocument.cpp:132
> > +        }
> 
> I think the local variable should just be named “widget”. Is there another widget other than then plug-in widget?
> 
> The definition of the local variable could be inside the if statement.

Fixed.

> 
> It’s not clear why setShouldBufferData(false) needs to be inside the if statement.

It doesn't need to be. Moved out of the if.

> 
> > WebCore/html/PluginDocument.h:47
> > +    bool pluginLoadWasCancelled() const { return m_pluginLoadWasCancelled; }
> > +    void setPluginLoadWasCancelled(bool wasCancelled) { m_pluginLoadWasCancelled = wasCancelled; }
> 
> Does this really need to be specific to plug-ins? I don’t see how the fact that a load was cancelled is a plug-in specific concept. Can this boolean be on DocumentLoader instead?

Well, in this case, it's not the load of the PluginDocument itself, it's of the load of the plugin inside of the plugin document. What this boolean is used for (whether or not to load the plugin inside of a plugin document manually) is a plug-in document specific concept. I could rename the function to be clearer about what this function does: Maybe something like setShouldLoadPluginManually/shouldLoadPluginManually.

[Brian Weinstein]

> > It’s not clear why setShouldBufferData(false) needs to be inside the if statement.
> 
> It doesn't need to be. Moved out of the if.

I was wrong about this, if it is moved out of the if:

frame->loader()->activeDocumentLoader()->mainResourceLoader()

is null, which causes us to crash later. This is because the main resource load has been cancelled. I could put it in its own null check (null checking mainResourceLoader(), or I could put it in the Widget* null check.

[Brian Weinstein]

Created attachment 69250 [details]
[PATCH] Fix w/ Darin's Comments Addressed

[Brady Eidson]

Comment on attachment 69250 [details]
[PATCH] Fix w/ Darin's Comments Addressed

I gave Brian some comments IRL (namely to add some comments to explain things), but am okay with it overall.

[Brian Weinstein]

Landed in r68702.

[Eric Seidel (no email)]

Looks like this needs to be skipped on Qt.