Closed Bug 306867 Opened 19 years ago Closed 8 years ago

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

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
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: 8 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.