846896 - Don't allow windowed plugins in popups even if we try to swap them into a document in a popup

Status: RESOLVED INVALID

(Core :: Web Painting, defect)

Description

[Timothy Nikkel (:tnikkel)]

Attachment: patch

In nsObjectFrame::PrepForDrawing we don't allow windowed plugins in popups. But if we swap a document into a popup it can still happen because we don't do any checks in nsObjectFrame::EndSwapDocShells.

We'll still have an inner view (PrepForDrawing would just no ever create one) so this isn't exactly what PrepForDrawing would do to disable the plugin.

The rendering is still a bit weird in the popup where the plugin should be after this patch but at least we don't have a plugin floating around randomly on screen.

Comment 1

[Mats Palmgren (inactive)]

Comment on attachment 720084 [details] [diff] [review]
patch

It seems too risky to just return and leave the widget's parent as is.

I think we need to take more drastic measures to get out of this situation.
How about calling nsObjectLoadingContent::StopPluginInstance() ?
That should destroy the nsPluginInstanceOwner + widget.

Comment 2

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren [:mats] from comment #1)
> I think we need to take more drastic measures to get out of this situation.
> How about calling nsObjectLoadingContent::StopPluginInstance() ?
> That should destroy the nsPluginInstanceOwner + widget.

Hmm, that is more drastic than what we do in PrepForDrawing when we encounter the same situation.

What if we destroyed the inner view and detached the event handler and undo whatever else PrepForDrawing would have skipping this case?

Comment 3

[Mats Palmgren (inactive)]

(In reply to Timothy Nikkel (:tn) from comment #2)
> Hmm, that is more drastic than what we do in PrepForDrawing when we
> encounter the same situation.

Indeed, I think PrepForDrawing should be more drastic too.
We intentionally do not support this state so I think we can be as drastic
as we like, including destroying the plugin/frame/widget/view...

> What if we destroyed the inner view and detached the event handler and undo
> whatever else PrepForDrawing would have skipping this case?

Yes, at least that.  We should also reparent the widget somehow.  I suggest
to "nsContentUtils::WidgetForDocument(content->OwnerDoc)" so that it mimics
what nsPluginInstanceOwner::CreateWidget() would do before it calls
PrepForDrawing().

Comment 4

[Timothy Nikkel (:tnikkel)]

One argument for not stopping the plugin instance is that plugins that only play audio will work just fine. So just disabling the display portion of the plugin would be best for those.

Comment 5

[Mats Palmgren (inactive)]

We're in the process of removing support for plugins (bug 1677160), so I think this bug is irrelevant now.