767633 - remove obsolete channel handling code from nsPluginHost

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[John Schoenick [:johns]]

nsPluginHost has primary channel opening logic that should be entirely superceded by nsObjectLoadingContent, and should be cleaned up and removed

Comment 1

[Josh Aas]

What is the plan here John?

Comment 2

[John Schoenick [:johns]]

Attachment: Part 1 - nsObjectLoadingContent should handle all initial streams for plugins

So nsPluginHost is still handling streams for plugins that instantiate without
considering the channel type. Opening the channel, then, is done by pluginHost -
which is ultimately another unnecessary codepath. This is also the codepath
that's leaking in bug 812629.

This just moves all initial stream opening to ObjectLoadingContent, without
changing the logic for selecting a plugin type.

Comment 3

[John Schoenick [:johns]]

Attachment: Part 2 - Remove a ton of obsolete nsPluginHost code

Removes the code obsoleted by Part 1, as well as a ton of code that was
obsoleted by refactoring ObjectLoadingContent, and a little that has been dead
since ObjectLoadingContent was created...

Comment 4

[John Schoenick [:johns]]

Attachment: Bonus - Clean up whitespace in nsPluginHost

Bonus whitespace changes

Comment 5

[Josh Aas]

Comment on attachment 686893 [details] [diff] [review]
Part 2 - Remove a ton of obsolete nsPluginHost code

Review of attachment 686893 [details] [diff] [review]:
-----------------------------------------------------------------

These patches are so beautiful, I think I'm tearing up a little.

Comment 6

[John Schoenick [:johns]]

https://tbpl.mozilla.org/?tree=Try&rev=1c5c0a2f0086

Comment 7

[Mozilla RelEng Bot]

Try run for 1c5c0a2f0086 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1c5c0a2f0086
Results (out of 125 total builds):
    exception: 1
    success: 83
    warnings: 25
    failure: 16
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1c5c0a2f0086

Comment 8

[John Schoenick [:johns]]

Attachment: Part 1 - Cleanup nsObjectLoadingContent logic a bit

Okay, new edition of this patch series.

This patch just cleans up some logic and comments, and should be a no-op
functionality wise.

Comment 9

[John Schoenick [:johns]]

Attachment: Part 2 - nsObjectLoadingContent should handle all initial streams for plugins

So we the previous version of this patch caused us to always wait on channels to
spawn plugins - but this regresses behavior of tags like:

> <embed src="./pony.swf">

Because we will see .swf, spawn flash, and then create a pluginstreamlistener
for it after the fact.

In this version of the patch we change ObjectLoadingContent to be able to handle
channels opened after the fact, so we can still "fast" spawn this type of
plugin. This also would make it possible to make <object> behave this way when
it has an explicit type, but I don't think we want to add more ways to
synchronously spawn plugins.

Compared to the earlier version of this patch, this makes OnStartRequest and
LoadObject aware of these 'late' streams for plugins, and adds a helper to
(re)create a nsPluginStreamListener for plugins that load this way, incidentally
fixing bug 767627.

Comment 10

[John Schoenick [:johns]]

Attachment: Part 3 - Remove a ton of obsolete nsPluginHost code. r=josh

Comment 11

[John Schoenick [:johns]]

Attachment: Part 4 - nsObjectLoadingContent - Don't erroneously decide state changed because the channel went away

This fixes a bug I uncovered with the previous changes - if we call LoadObject
(such as because we re-bound to tree) after mChannel has hit OnStopRequest(),
we'll think we need to open a new channel and re-load because we can't inspect
its content type.

This adds a few checks to just re-use the existing type if our state didn't
otherwise change.

Comment 12

[John Schoenick [:johns]]

Attachment: Bonus - Clean up whitespace in nsPluginHost. r=josh

Comment 13

[John Schoenick [:johns]]

https://tbpl.mozilla.org/?tree=Try&rev=1c691ae2aca7

Comment 14

[Josh Aas]

Comment on attachment 690640 [details] [diff] [review]
Part 2 - nsObjectLoadingContent should handle all initial streams for plugins

Review of attachment 690640 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsPluginHost.h
@@ -211,5 @@
>    nsresult
>    TrySetUpPluginInstance(const char *aMimeType, nsIURI *aURL, nsPluginInstanceOwner *aOwner);
>  
> -  nsresult
> -  NewEmbeddedPluginStream(nsIURI* aURL, nsObjectLoadingContent *aContent, nsNPAPIPluginInstance* aInstance);

I was confused about this at first but it seems you already removed the method and its callers in a previous patch, and just forgot this.

Comment 15

[John Schoenick [:johns]]

Attachment: Part 2 - nsObjectLoadingContent should handle all initial streams for plugins. r=josh

Folded removing NewEmbeddedPluginStream from the header into the proper patch (part 3)

Comment 16

[John Schoenick [:johns]]

Attachment: Part 3 - Remove a ton of obsolete nsPluginHost code. r=josh

Comment 17

[John Schoenick [:johns]]

Comment on attachment 690637 [details] [diff] [review]
Part 1 - Cleanup nsObjectLoadingContent logic a bit

https://hg.mozilla.org/integration/mozilla-inbound/rev/6aed5f5caecb
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b68ff48e22c
https://hg.mozilla.org/integration/mozilla-inbound/rev/11341616d7cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2476e368fb6
https://hg.mozilla.org/integration/mozilla-inbound/rev/62389ec6712c

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6aed5f5caecb
https://hg.mozilla.org/mozilla-central/rev/1b68ff48e22c
https://hg.mozilla.org/mozilla-central/rev/11341616d7cb
https://hg.mozilla.org/mozilla-central/rev/a2476e368fb6
https://hg.mozilla.org/mozilla-central/rev/62389ec6712c

Comment 19

[neils]

Thanks for the fixes!

Is Mozilla20 the sames as Firefox 20?

When can users download the updated/corrected version?

Thanks in advance,
Neil

Comment 20

[Andrew McCreight [:mccr8]]

It is available for testing in the next day or two on the experimental Nightly Channel. Firefox 20 will move to Aurora, and later to Beta. Firefox 20 is scheduled to be released to the general public on April 2. (See https://wiki.mozilla.org/RapidRelease/Calendar .)

Comment 21

[neils]

I tried the nightly build today, and the moment I hit the 3D Stereo option to activate the plug-in, Firefox crashes to desktop.  Do you know if the bug has been fixed in the currently available nightly build?

Can you do a quick test of the plug-in to see if it works for you?

Thanks in advance,
Neil

Comment 22

[John Schoenick [:johns]]

(In reply to neils from comment #21)
> I tried the nightly build today, and the moment I hit the 3D Stereo option
> to activate the plug-in, Firefox crashes to desktop.  Do you know if the bug
> has been fixed in the currently available nightly build?
> 
> Can you do a quick test of the plug-in to see if it works for you?
> 
> Thanks in advance,
> Neil

This is probably unrelated to the changes in this bug, can you open a new bug report for this?

Comment 23

[Mozilla RelEng Bot]

Try run for 1c5c0a2f0086 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1c5c0a2f0086
Results (out of 126 total builds):
    exception: 1
    success: 83
    warnings: 25
    failure: 17
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1c5c0a2f0086