WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
46773
REGRESSION (
r66129
): Loading full-frame .swf file crashes with flash blocker extension enabled
https://bugs.webkit.org/show_bug.cgi?id=46773
Summary
REGRESSION (r66129): Loading full-frame .swf file crashes with flash blocker ...
Brian Weinstein
Reported
2010-09-28 18:56:45 PDT
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.
Attachments
[PATCH] Fix
(6.89 KB, patch)
2010-09-28 19:07 PDT
,
Brian Weinstein
bweinstein
: commit-queue-
Details
Formatted Diff
Diff
[PATCH] Fix with tests
(10.80 KB, patch)
2010-09-29 11:29 PDT
,
Brian Weinstein
bweinstein
: commit-queue-
Details
Formatted Diff
Diff
[PATCH] Fix w/ Darin's Comments Addressed
(11.56 KB, patch)
2010-09-29 14:27 PDT
,
Brian Weinstein
beidson
: review+
bweinstein
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Brian Weinstein
Comment 1
2010-09-28 18:56:59 PDT
<
rdar://problem/8390975
>
Brian Weinstein
Comment 2
2010-09-28 19:07:50 PDT
Created
attachment 69152
[details]
[PATCH] Fix
Adam Roben (:aroben)
Comment 3
2010-09-29 07:47:24 PDT
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
Comment 4
2010-09-29 09:41:47 PDT
(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
Comment 5
2010-09-29 11:29:03 PDT
Created
attachment 69231
[details]
[PATCH] Fix with tests
Darin Adler
Comment 6
2010-09-29 12:45:02 PDT
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
Comment 7
2010-09-29 13:32:03 PDT
(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
Comment 8
2010-09-29 13:52:55 PDT
> > 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
Comment 9
2010-09-29 14:27:00 PDT
Created
attachment 69250
[details]
[PATCH] Fix w/ Darin's Comments Addressed
Brady Eidson
Comment 10
2010-09-29 14:32:47 PDT
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
Comment 11
2010-09-29 16:01:22 PDT
Landed in
r68702
.
Eric Seidel (no email)
Comment 12
2010-09-29 20:19:12 PDT
Looks like this needs to be skipped on Qt.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug