751237 - FF fails to use another plug-in (or asking user) when first plugin returns error from NP_Initialize

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Plug-ins, defect)

Description

[Tom Watson]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:11.0) Gecko/20100101 Firefox/11.0
Build ID: 20120312181643

Steps to reproduce:

I modified the AdobePDFViewerNPAPI plugin to return an error from NP_Initialize when running in 64-bits (as we currently only support 32-bit). 


Actual results:

FF12 properly shut our plug-in down and then (improperly) displayed a blank page.


Expected results:

FF12 should have resorted to another plug-in with PDF MIME type support (I tried putting a second in the plug-ins directory.  Or, if no alternate plug-in, FF12 should have asked the user to choose an app or save the file as it does when there is no plug-in associated with a MIME type.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

I'll get to this today or tomorrow.

Right now I'm investigating something that's even more urgent (wrt app signing on Mountain Lion).

Comment 2

[Steven Michaud [:smichaud] (Retired)]

I've confirmed this, using a debug build where I pretended that the current version (10.1.3) of the Adobe Reader plugin returned an error from NP_Initialize() in 64-bit mode.  Firefox didn't fail over to the next MIME handler.

The same thing happened if I emulated an error return from NP_GetEntryPoints() or NPP_New() -- no failover.

I'll be looking for a quick fix, which I hope to have available by early next week.

I'm told that John Schoenick is working on a refactoring of plugin code that's likely to fix this problem.  When that's done we can alter/remove my quick fix as appropriate.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Attachment: Preliminary fix

It took me longer to find my "quick fix" than I'd hoped, and my patch has one significant problem.

But the patch does fix this bug in the simplest case -- it falls back to using a helper app when a plugin returns an error from NP_Initialize() or NP_GetEntryPoints().

It *would* also allow failing over from one plugin to another, except for a bug (or design flaw) in the category manager:  If two or more plugins register the same mime type with the category manager, it only gets registered once.  So then if one of these plugins gets disabled or "breaks" (returns an error from NP_Initialize() or NP_GetEntryPoints()), then it and all the other plugin handlers of that mime type get unregistered at the same time.

I'm not sure what to do about the category manager.  I'll look further into that on Monday.

In the meantime people can test with my current patch.  I've started a tryserver build, which should be available by tomorrow morning.

This patch (and the eventual tryserver build) are intended for Adobe to test wit newer versions of their Adobe Reader plugin -- ones that return an error from NP_Initialize().  In my next comment I'll post a debugging patch that can be used to test with the current version of Adobe Reader (10.1.3) -- it emulates an error return from NP_Initialize() whenever the plugin's filename contains the string "AdobePDFViewerNPAPI".

My patch adds support for the notion of a "broken" plugin -- one that returns an error from one of the initialization routines (NP_Initialize() or NP_GetEntryPoints()).  A broken plugin is (in effect) disabled from the point that it "breaks" until you restart the browser.  But then you start over with a fresh slate -- a plugin's "brokenness" isn't stored in pluginreg.dat.  Among other things, this accommodates a plugin that works in 32-bit mode but not in 64-bit mode.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Attachment: Debugging patch

Comment 5

[Steven Michaud [:smichaud] (Retired)]

It occurs to me that we'd also want to display a plugin's broken state in the Add-Ons Manager (i.e. enabled but broken).

With my current patch the broken plugin appears there but is just marked enabled.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made from my patch from comment #3:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ec612de54f0c/try-macosx64/firefox-15.0a1.en-US.mac.dmg

Here's one made from my debugging patch from comment #4:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-8997088f5d3e/try-macosx64/firefox-15.0a1.en-US.mac.dmg

There were no non-spurious test failures.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 623377 [details] [diff] [review]
Preliminary fix

John, what do you think of this approach?

Does it conflict with the refactoring work you're doing at bug 745030?

Have you run into the problem with the category manager yet (see comment #3)?  And if so do you have any ideas about how we should fix it (or work around it)?

Comment 8

[John Schoenick [:johns]]

Comment on attachment 623377 [details] [diff] [review]
Preliminary fix

I'm not sure if letting the broken plugin error bubble up through OnStartRequest and then retry in the URI loader is the best way to retry. It seems that pluginHost should transparently retry until it runs out of potential plugins, and only then return the broken plugin error. ObjectLoadingContent could handle that error then in its passing to mFinalListener->OnStartRequest(). It's also possible for ObjectLoadingContent to instantiate a plugin without a channel, where we would need to catch the broken plugin error as well.

Bug 745030 should be landing this cycle, so one option could be just landing the pluginHost/NPAPI changes for returning NS_ERROR_PLUGIN_BROKEN, I could make sure the new objLC handles them

Comment 9

[John Schoenick [:johns]]

Sorry, I reread your note about comment #3 and multiple plugins -- I haven't run into that, it seems like something will need to change there, but supporting multiple broken plugins per MIME type is probably a lot less of a concern

Comment 10

[Steven Michaud [:smichaud] (Retired)]

> but supporting multiple broken plugins per MIME type is probably a lot less of a
> concern

I'm talking about failing over from one plugin that handles mimetype "foo" to another plugin that handles mimetype "foo".  This is what the problem with the category manager prevents from working.

Comment 11

[John Schoenick [:johns]]

That's something that probably needs to be fixed then, but I haven't messed much with the category manager yet, so I can't be of much help there. Josh might have some experience with that code

Comment 12

[Steven Michaud [:smichaud] (Retired)]

The category manager comes into play at nsPluginTag::RegisterWithCategoryManager():
http://hg.mozilla.org/mozilla-central/annotate/498d2784a240/dom/plugins/base/nsPluginTags.cpp#l363

Comment 13

[Steven Michaud [:smichaud] (Retired)]

Adobe people:

I'm no longer sure it makes sense to address this problem separately from the work John Schoenick is doing at bug 745030, and that patch (being quite complex) may take a while to get into the tree.  I suspect it's a bit optimistic to say bug 745030 will be "landing this cycle" :-)

So you guys need to start work on Josh's other suggestion.  Quoting from email (which I assume Josh won't mind, since almost all parties here were CCed on the message):

"The 64-bit problem can be solved pretty easily with some basic drawing code (fill in the display rect), possibly a textual indicator of what is going on, and then hand the file off to the OS (preferring the installed Reader app). All of this is probably a combined < 100 lines of code. There is a bunch of sample code for this sort of basic drawing in the npapi-sdk sample plugin for Mac OS X."

Comment 14

[John Schoenick [:johns]]

Attachment: The plugin category manager should account for multiple plugins per MIME

Fix to category manager code that should avoid incorrectly unregistering plugins when multiple claim a MIME

There is still a GUI issue where you cannot select *which* plugin to use in the applications preferences - Though the drop down shows a plugin name, what the preference is actually showing is "use any plugin" or "use something else"

Comment 15

[John Schoenick [:johns]]

I should note - this lands on top of the patch in bug 755551, which adds the IsTypeInPrefList function

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 638499 [details] [diff] [review]
The plugin category manager should account for multiple plugins per MIME

This patch doesn't fix this bug.  Nor does it address any of this bug's fundamental issues:

It doesn't add support for failing over from a plugin to a helper app.  (And I don't know how this could be done without "letting the broken plugin error bubble up through OnStartRequest").

It doesn't add support for failing over from one plugin to another.

The only thing it *does* do is make it possible to disable a plugin without unregistering all that plugin's MIME types with the Category Manager.  (Which means that on subsequent attempts to find a handler for a "broken" plugin's MIME type, the Category Manager will keep looking for a plugin to handle the MIME type.)  I suppose this is a step in the right direction (though a very small one).  So I'll approve this patch.

But there's a lot more work to do here.

Comment 17

[John Schoenick [:johns]]

(In reply to Steven Michaud from comment #16)
> Comment on attachment 638499 [details] [diff] [review]
> The plugin category manager should account for multiple plugins per MIME
> 
> This patch doesn't fix this bug.  Nor does it address any of this bug's
> fundamental issues:
> 
> It doesn't add support for failing over from a plugin to a helper app.  (And
> I don't know how this could be done without "letting the broken plugin error
> bubble up through OnStartRequest").
> 
> It doesn't add support for failing over from one plugin to another.
> 
> The only thing it *does* do is make it possible to disable a plugin without
> unregistering all that plugin's MIME types with the Category Manager. 
> (Which means that on subsequent attempts to find a handler for a "broken"
> plugin's MIME type, the Category Manager will keep looking for a plugin to
> handle the MIME type.)  I suppose this is a step in the right direction
> (though a very small one).  So I'll approve this patch.
> 
> But there's a lot more work to do here.

I'm aware that this is only a small part of the problem - I just wanted to get your feedback on the category manager issue, as I wrote this in conjunction with changes made in 755551

The proper way to fix this bug would be your patch rebased on top of bug 745030 (now pending review), with a few changes to the spot where we retry on broken as I noted in c8

Comment 18

[Steven Michaud [:smichaud] (Retired)]

OK, then.

I'll wait for your patch to bug 745030 to land, and for the additional changes (which will I assume be landed in another bug).

Thanks for your work on this!

Comment 19

[John Schoenick [:johns]]

Attachment: The plugin category manager registration should account for multiple plugins per MIME

Comment 20

[John Schoenick [:johns]]

Comment on attachment 640709 [details] [diff] [review]
The plugin category manager registration should account for multiple plugins per MIME

Bah, attached to the wrong bug

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Is there anything left to do here now that bug 745030 and bug 772590 are resolved?

Comment 22

[Steven Michaud [:smichaud] (Retired)]

> Is there anything left to do here now that bug 745030 and bug 772590 are resolved?

Yes, quite a lot.  See comment #16.

(I haven't yet tested with the patch for bug 745030, but I don't think it addresses anything in this bug.  Though both it and bug 772590 will (I hope) make it easier to fix this bug.)

Comment 23

[Benjamin Smedberg]

I don't understand why this bug is marked tracking. It's not a regression nor does it seem to be an especially urgent issue, at least as I'm reading it.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #23)

A lot depends on whether Adobe is willing to follow the advice outlined in comment #13 :-)

Comment 25

[John Schoenick [:johns]]

As for fixing this now that 745030 is landed, I think the "Preliminary Fix" patch is a good approach here, but the spot that we retry when plugins are broken should change from nsURILoader.cpp to nsObjectLoadingContent (we should re-trigger LoadObject when instantiation fails, so it can try a new plugin or load fallback content).

One approach would be rebasing the patch here minus the URILoader change, and then opening a followup bug to properly handle the plugin-broken flag in nsObjectLoadingContent. I can implement the latter pretty easily along with the bug 767635 changes I'm working on.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

I'm not sure it'd be possible to implement support for failing over from a ("broken") plugin to a helper app without the URILoader changes (or something like them).

And that's the most urgent issue here (thanks to Adobe's apparent desire to depend on it).

It'd be also nice to be able to fail over from one plugin to another, but that's less urgent (since, as far as I know, Adobe won't need it).

Is there any chance, John, you could handle both issues?  If so, I'd be more than happy to pass this bug off to you :-)

Comment 27

[John Schoenick [:johns]]

(In reply to Steven Michaud from comment #26)
> I'm not sure it'd be possible to implement support for failing over from a
> ("broken") plugin to a helper app without the URILoader changes (or
> something like them).
> 
> And that's the most urgent issue here (thanks to Adobe's apparent desire to
> depend on it).

With the bug 745030 changes, helper app fallback should work in the non-full-page context, e.g. <object> tags, as we would re-trigger the LoadObject() sequence, find no plugin, and try to load as a subdocument (and if that doesn't work, it's yet-another-bug)

For full-page documents you're right that this wouldn't work, though perhaps after bug 767638 it would work in roundabout way -- that bug will make full page plugins more similar to simple pages with a single <object> tag, which would result in said object tag falling back to a helper. Perhaps we could even add a shim where if we fallback to a document type inside a PluginDocument page, we just navigate directly to that document.

> It'd be also nice to be able to fail over from one plugin to another, but
> that's less urgent (since, as far as I know, Adobe won't need it).

That part, at least, should just-work with this patch + retriggering LoadObject

> Is there any chance, John, you could handle both issues?  If so, I'd be more
> than happy to pass this bug off to you :-)

I can take over this bug, but I think the easiest way forward would be to spin off a "add broken plugin tag support" bug and land your patch minus the URILoader change, then work incrementally from there to make the various plugin-loading contexts respect it

Comment 28

[Steven Michaud [:smichaud] (Retired)]

> but I think the easiest way forward would be to spin off a "add
> broken plugin tag support" bug and land your patch minus the
> URILoader change, ...

I'll try this, probably sometime next week.

Comment 29

[Benjamin Smedberg]

Resolving old bugs which are likely not relevant any more, since NPAPI plugins are deprecated.