Closed Bug 306867 Opened 19 years ago Closed 7 years ago

No way for plugin to not handle NP_FULL when it handles the mime-type

Categories

(Core Graveyard :: Plug-ins, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: bugzilla, Assigned: edburns)

Details

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050719 Epiphany/1.6.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050719 Epiphany/1.6.5

A plugin has no way to not handle a particular plugin mode (from ->newp).

In the new instance function, it should be possible to return a
NPERR_MODE_UNSUPPORTED if the mode doesn't match a supported one (for example, a
video plugin that doesn't want to handle NP_FULL).

Reproducible: Always
Christian Persch in http://bugzilla.gnome.org/show_bug.cgi?id=314370 (for Totem) notes:
"
The exact error code doesn't matter, gecko just checks for != NPERR_NO_ERROR.
However returning an error doesn't seem to work here, it still does not pass
the URL to the ext handler.
"
Be nice to get this fixed for Fx2, but we'll need some hustle on it.

Bastien/chpe: is there a simple plugin anywhere that could help us isolate and verify a fix for this?  Something we could build from source would be especially helpful, since I'm not sure what platform will be relevant when I find a victimteer to take a look. :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
I believe the easiest to build would be VideoLAN's Mozilla plugin (it works on Windows and Mac as well as Unices)

This is the Unix version:
http://trac.videolan.org/vlc/browser/trunk/mozilla/support/npunix.c

Add those lines to Private_New should be enough to check the behaviour:
if (mode == NP_FULL)
  return NPERR_INVALID_PLUGIN_ERROR;

For the actual test, have a file available on a website that will be of a mime-type supported by the plugin (such as "audio/wav"[1]) and browse directly to the file itself. The browser will play it back using the whole canvas (without the modifications to the plugin above), or not do anything (with the modifications above).

[1]: http://trac.videolan.org/vlc/browser/trunk/mozilla/vlcplugin.h#L108
Hello Bastien,

Is http://trac.videolan.org/vlc/browser/trunk/mozilla/support/npwin.cpp known to work on Win32?  

I can take a stab at fixing this if I am able to build your example plugin on win32.
Assignee: nobody → edburns
Ed: build from MOZILLA_1_8_BRANCH to get started on this.
Status: NEW → ASSIGNED
(In reply to comment #4)
> Hello Bastien,
> 
> Is http://trac.videolan.org/vlc/browser/trunk/mozilla/support/npwin.cpp known
> to work on Win32?  
> 
> I can take a stab at fixing this if I am able to build your example plugin on
> win32.

I'm afraid I don't know the status of this plugin (it's not the application I hack on ;).
Maybe it would be easier to copy the nullplugin, and make it handle a simple mime-type, like "text/plain", and see whether Mozilla/Firefox offers to download the file when accessing a text/plain mime-type file on its own, or whether it will try and load the new hacked plugin.


text/plain would be a bad choice, probably, but some type that's handled by an helper app.  application/x-ms-word? :)

Bastien: do you think you could take a swing at that nullplugin hack you describe?  Ed's going to try to get a fix in time for me to risk my life pushing to land it on the Fx2 branch, but any help we can get to accelerate things would improve our chances of timely success.
(In reply to comment #7)
> text/plain would be a bad choice, probably, but some type that's handled by an
> helper app.  application/x-ms-word? :)

Yeah, forgot that text/plain was handled internally...

> Bastien: do you think you could take a swing at that nullplugin hack you
> describe?  Ed's going to try to get a fix in time for me to risk my life
> pushing to land it on the Fx2 branch, but any help we can get to accelerate
> things would improve our chances of timely success.

The plugin hack I described is a one-liner, ie. changing the handled mime-types from "*" to "application/x-ms-word". I could do a diff, but that would just be ridiculous.
Iirc the nullplugin is unix-only too...
The test plugin in bug 320888 might be a starting place to add this (I developed it on unix but it doesn't use anything unix-specific, should compile on win32 too.)
Thanks for the test plugin Christian.
The Plugin SDK has some samples.  I already can build that.  Let me make sure I have it straight.  We need a way for a plugin to say, "Nope, I don't do that mode," even though it *does* do that content type?

The test environment is to hack a buildable plugin to handle, say, application/x-ms-word, but don't have it do the full screen mode.  Then, write some HTML that calls the plugin *in full screen mode* and make sure that the browser calls the external helper app.

Is that the idea?
Ok, I can reproduce the problem by modifying npsimple.rc to support application/msword, then doing file->Open and choosing a word file.  My plugin gets called with mode == NP_FULL.  I added a new constant to npapi.h, NPERR_MODE_UNSUPPORTED.  I made the hacked npsimple return that if mode == NP_FULL.  This causes the browser to show a white page and not offer to spawn the external viewer, which *does* happen if the hacked npsimple.dll is not in place and I visit the word file.

I will take the following approach to fix the bug.

Remove the hacked npsimple.dll. 

Discover the codepath that results in the external viewer dialog and see if it intersects with the "launch a plugin" codepath.  I'm thinking it does.  

At the point in time when the plugin codepath is executed, add a check for the return of NPERR_MODE_UNSUPPORTED, and if true, continue to spawn the external viewer.

Please critique my approach.

Sincerely,

Ed
Ok, this is the codepath that causes the external helper dialog to open.

nsExternalAppHandler::OnStartRequest(nsIRequest * 0x03d0a2a8, nsISupports * 0x00000000) line 1784
nsDocumentOpenInfo::OnStartRequest(nsDocumentOpenInfo * const 0x03d0a528, nsIRequest * 0x03d0a2a8, nsISupports * 0x00000000) line 356 + 15 bytes
nsFileChannel::OnStartRequest(nsFileChannel * const 0x03d0a2b0, nsIRequest * 0x03e22818, nsISupports * 0x00000000) line 539 + 66 bytes
nsInputStreamPump::OnStateStart(nsInputStreamPump * const 0x001efb34) line 442
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x001a075e, nsIAsyncInputStream * 0x03e2281c) line 404
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x03e228b4) line 120 + 12 bytes
PL_HandleEvent(PLEvent * 0x03e228b4) line 689
PL_ProcessPendingEvents(PLEventQueue * 0x001b518b) line 624
nsEventQueueImpl::ProcessPendingEvents(nsEventQueueImpl * const 0x01132348) line 421
nsWindow::DispatchPendingEvents(nsWindow * const 0x001efb34) line 4410
nsWindow::ProcessMessage(nsWindow * const 0x001efb34, unsigned int 512, unsigned int 0, long 16908958, long * 0x00000001) line 4898
nsWindow::WindowProc(HWND__ * 0x00020320, unsigned int 512, unsigned int 0, long 51730484) line 1531 + 16 bytes
USER32! 77e4158f()
USER32! 77e41dc9()
USER32! 77e41e7e()
nsAppStartup::Run(nsAppStartup * const 0x011e69e0) line 152
XRE_main(int 31062856, char * * 0x0084f3bc, const nsXREAppData * 0x011e69e0) line 2440
main(int 1, char * * 0x008489e0) line 61 + 18 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c598989()
Ok, I have found that nsDocumentOpenInfo::OnStartRequest() gets called regardless of whether or not the plugin is installed.  Furthermore, the call to NPP_New happens *after* the nsDocumentOpenInfo::OnStartRequest() call.

I'm honing in on the right place in the code.
It looks like the Strategy pattern is being employed by nsDocumentOpenInfo::OnStartRequest() to find the appropriate m_targetStreamListener instance.  In the case of a plugin, it is an instance of nsMediaDocumentStreamListener.  In the case of a helper app, it is nsExternalAppHandler.

The thing to do now is to see if I can recover the error return from the plugin from within nsDocumentOpenInfo::OnStartRequest(), so I can select a different strategy in the case where the plugin doesn't support the mode.
I'll resume with nsDocumentOpenInfo::DispatchContent().
Good work Ed.
I require feedback on the following implementation path.  I feel the following strategy is NOT acceptable for this bug.

Define a new value queried by NPP_GetValue() that allows the browser to ask the plugin are you a full page plugin.

This strategy will not work for several reasons:

* The full vs non-full determination can only be made when the plugin is given the content type and perhaps some other parameters.  This information can not be conveyed using NPP_GetValue().

* The plugin may want to display in different modes based on the arguments to NPP_New.
Here is an implementation strategy that I think will work.

1. Modify the return value of the call chain from
   nsObjectFrame::InstantiatePlugin() on down to
   ns4XPluginInstance::InitializePlugin() so that the new
   NPERR_MODE_UNSUPPORTED value is returned to
   nsObjectFrame::InstantiatePlugin() (lines 1359 and 1369).

2. Introduce a new attribute on nsIPluginHost, 

   boolean lastInstantiationWasSuccessful

3. Modify nsPluginHostImpl::Instantiate{FullPage,Embedded}Plugin() to
   set the value of the lastInstantiationWasSuccessful attribute based
   on whether or not this instantiation attempt was successful.

This extra attribute, while hacky, is necessary because the return
status of NPP_New is lost on the callstack between the actuall call to
NPP_New, and the decision point where it is convenient to call the
helper app service.  Here's that callstack, just in case you don't believe me.

NPP_New() line 55
ns4xPluginInstance::InitializePlugin() line 1086 + 42 bytes
ns4xPluginInstance::Initialize() line 836
nsPluginHostImpl::TrySetUpPluginInstance() line 3959
nsPluginHostImpl::SetUpPluginInstance() line 3770
nsPluginHostImpl::InstantiateFullPagePlugin() line 3571
nsObjectFrame::InstantiatePlugin() line 1361 + 30 bytes
nsObjectFrame::Reflow() line 1239 + 25 bytes
nsLineLayout::ReflowFrame() line 996
nsBlockFrame::ReflowInlineFrame() line 4042
nsBlockFrame::DoReflowInlineFrames() line 3880
nsBlockFrame::ReflowInlineFrames() line 3762
nsBlockFrame::ReflowLine() line 2756
nsBlockFrame::ReflowDirtyLines() line 2290
nsBlockFrame::Reflow() line 903
nsBlockReflowContext::ReflowBlock() line 606
nsBlockFrame::ReflowBlockFrame() line 3476 + 55 bytes
nsBlockFrame::ReflowLine() line 2637 + 20 bytes
nsBlockFrame::ReflowDirtyLines() line 2290
nsBlockFrame::Reflow() line 903
nsContainerFrame::ReflowChild() line 905 + 20 bytes
CanvasFrame::Reflow() line 536
nsContainerFrame::ReflowChild() line 905 + 20 bytes
nsHTMLScrollFrame::ReflowScrolledFrame() line 523
nsHTMLScrollFrame::ReflowContents() line 571
nsHTMLScrollFrame::Reflow() line 769
nsContainerFrame::ReflowChild() line 905 + 20 bytes
ViewportFrame::Reflow() line 240
PresShell::InitialReflow() line 2905
nsMediaDocument::StartLayout() line 280
nsMediaDocumentStreamListener::OnStartRequest() line 84
nsDocumentOpenInfo::OnStartRequest() line 356 + 15 bytes
nsFileChannel::OnStartRequest() line 539 + 66 bytes
nsInputStreamPump::OnStateStart() line 442
nsInputStreamPump::OnInputStreamReady() line 404
nsInputStreamReadyEvent::EventHandler() line 120 + 12 bytes
PL_HandleEvent() line 689
PL_ProcessPendingEvents() line 624
_md_TimerProc() line 1013 + 6 bytes
USER32! 77e4158f()
USER32! 77e3c01a()
USER32! 77e41e7e()
nsAppStartup::Run() line 152
XRE_main() line 2440
main() line 61 + 18 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 7c598989()

As far as nsMediaDocument::StartLayout() is concerned, the layout is
successful regardless of whether or not the plugin was successfully
instantiated.

5. Back up in nsDocumentOpenInfo::OnStartRequest(), after the call to
   m_targetStreamListener->OnStartRequest(), we add a call to a new
   private method, ShouldDispatchToHelperAppService() which "returns" a
   boolean if the helper app service should be called.  This will be
   true if the content was targeted at a plugin, *and* the plugin was
   not successfully instantiated.  

6. If ShouldDispatchToHelperAppService() "returned" true, we make
   m_targetStreamListener() point to the proper value for the helper app
   service, then invoke its onStartRequest() as normal.
That looks like a branch stack fwiw.

Why is it desirable that plugins can only handle inline content?
It's not that they can *only* handle inline content.  It's that they have the choice to support a particular mime-type + mode combination.  For example, a plugin may want to handle application/msword, but only in embedded mode.  Alternatively, a plugin may want to handle text/vrml but only in full screen mode.   Before this feature was contemplated, there was no way to de-couple the two concepts of mime-type and plugin mode.

Also, yes, this is a branch stack.  That's what Mike Shaver recommended I build.
well, I don't really like it that a certain type of content is only viewable when inline, or only when not inline, but ok...

The trunk code for this changed considerably. This is probably easier to fix on trunk. Be prepared for a lot of fun merging if you want this on both trunk and branch.
Adding a new NPERR code is probably not what we want to do, but just pretending the plugin wasn't there at all if it can't render it sounds like the right thing otherwise.
Ed, do you have any news on the patch?
So are we basically trying to treat the "plug-in can't handle" case just like the "have no plug-in" case?  In that case, we want to reenter dispatch, not just dispatch to the helper app service.  Dispatch tries several things between docshells (which is where plug-ins fit in) and the helper app service.

I seem to recall that biesi had a patch somewhere for allowing redispatch...
(In reply to comment #25)
> So are we basically trying to treat the "plug-in can't handle" case just like
> the "have no plug-in" case?  In that case, we want to reenter dispatch, not

Yes, exactly.

> just dispatch to the helper app service.  Dispatch tries several things between
> docshells (which is where plug-ins fit in) and the helper app service.
> 
> I seem to recall that biesi had a patch somewhere for allowing redispatch...

Could you add him to the CC:? I'd really like to see that bug fixed...
He's on the cc already. See comment 20 and comment 22.
the patch I had only allowed redispatch for the helper app service, not for other content listeners. fixing that might be possible, but it seems like you could get infinite loops that way.
Is there any progress on that bug?
can't be just disallow full page plugins (or make this configurable?)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.