remove obsolete channel handling code from nsPluginHost

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: johns, Assigned: johns)

Tracking

(Depends on 1 bug, Blocks 1 bug)

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 5 obsolete attachments)

14.95 KB, patch
jaas
: review+
johns
: checkin+
Details | Diff | Splinter Review
4.96 KB, patch
jaas
: review+
johns
: checkin+
Details | Diff | Splinter Review
14.47 KB, patch
johns
: checkin+
Details | Diff | Splinter Review
30.30 KB, patch
johns
: checkin+
Details | Diff | Splinter Review
10.19 KB, patch
johns
: checkin+
Details | Diff | Splinter Review
nsPluginHost has primary channel opening logic that should be entirely superceded by nsObjectLoadingContent, and should be cleaned up and removed
Blocks: 736998
What is the plan here John?
Blocks: 812629
Depends on: 767627
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.
Attachment #686891 - Flags: review?(joshmoz)
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...
Attachment #686893 - Flags: review?(joshmoz)
Bonus whitespace changes
Attachment #686894 - Flags: review?(joshmoz)
Attachment #686891 - Flags: review?(joshmoz) → review+
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.
Attachment #686893 - Flags: review?(joshmoz) → review+
Attachment #686894 - Flags: review?(joshmoz) → review+
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/jschoenick@mozilla.com-1c5c0a2f0086
Blocks: 818785
No longer depends on: 745030
Blocks: 818802
No longer blocks: 818802
Blocks: 818802
Okay, new edition of this patch series.

This patch just cleans up some logic and comments, and should be a no-op
functionality wise.
Attachment #690637 - Flags: review?(joshmoz)
Attachment #690637 - Attachment description: Bug 767633 - Part 1 - Cleanup nsObjectLoadingContent logic a bit → Part 1 - Cleanup nsObjectLoadingContent logic a bit
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.
Attachment #686891 - Attachment is obsolete: true
Attachment #690640 - Flags: review?(joshmoz)
Attachment #686893 - Attachment is obsolete: true
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.
Attachment #690645 - Flags: review?(joshmoz)
Attachment #690637 - Flags: review?(joshmoz) → review+
Blocks: 767638
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.
Attachment #690640 - Flags: review?(joshmoz) → review+
Attachment #690645 - Flags: review?(joshmoz) → review+
Folded removing NewEmbeddedPluginStream from the header into the proper patch (part 3)
Attachment #690640 - Attachment is obsolete: true
Attachment #691576 - Flags: checkin+
Attachment #691578 - Flags: checkin+
Attachment #690645 - Flags: checkin+
Attachment #690647 - Flags: checkin+
Thanks for the fixes!

Is Mozilla20 the sames as Firefox 20?

When can users download the updated/corrected version?

Thanks in advance,
Neil
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 .)
Depends on: 821843
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
(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?
Depends on: 822040
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/jschoenick@mozilla.com-1c5c0a2f0086
Depends on: 832032
Depends on: 860037
Depends on: 870216
You need to log in before you can comment on or make changes to this bug.