639647 - Restore Java scheme whitelist for NPRuntime plugins on the 1.9.2 branch

Status: RESOLVED WONTFIX

(Core Graveyard :: Plug-ins, defect)

Description

[Steven Michaud [:smichaud] (Retired)]

Shortly after FF 3.6.14 was released, people discovered that it broke
many Java plugins.  This breakage was triggered by my patch for bug
611910.

Since a fix was needed quickly, and because the problem fixed by my
patch for bug 611910 was much less serious than the problem it
triggered, it was decided to back out my patch (plus the patch for bug
620773, which depended on it) on the 1.9.2 branch.  This change was
included in FF 3.6.15, and was (as far as I know) the only change
included in that version.

For more information see bug 629030.

In the time since all this happened, I've discovered that bug 629030's
breakage was caused by a problem that happens only on the 1.9.2
branch, and has nothing to do with Java:  My original patch was broken
by how the browser re-uses plugin instances when reloading the same
kind of plugin.

My 1.9.2-branch patch for bug 611910 added an mPlugin member variable
to the nsNPAPIPluginInstance class.  This is a weak reference to the
nsNPAPIPlugin object of which it's an instance.

mPlugin is already present on the trunk, and causes no problems there.
On the trunk it's set in a constructor, then reset to NULL on calls to
nsNPAPIPluginInstance::Destroy().  But the 1.9.2 branch doesn't use
nsNPAPIPluginInstance::Destroy(), so my original patch reset it to
NULL on calls to nsNPAPIPluginInstance::Stop().  This was a mistake:
When a plugin instance is re-used, Stop() has already been called on
it, and its mPlugin member variable is NULL.  So my patch should have
re-initialized mPlugin on re-use, but it didn't do this.

Tomorrow I'll post a patch that restores my patches for bug 611910 and
620773 on the 1.9.2 branch, and also re-initializes mPlugin on re-use.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

Here's my promised patch.

Here also is a Windows build made from it:
https://people.mozilla.com/~stmichaud/bmo/firefox-3.6.16pre.en-US.win32-bugzilla639647.zip

Please try it out!  And let us know your results.

Comment 2

[Josh Aas]

I think this patch has pretty high inherent risk. Are we sure we still want to take this on 1.9.2? How many crashes does this fix?

Comment 3

[Steven Michaud [:smichaud] (Retired)]

> I think this patch has pretty high inherent risk.

Really?  My patch for bug 611910 has been on the trunk, the 1.9.1 branch and the 2.0 branch for quite a while without causing trouble.

> How many crashes does this fix?

Not many.

Comment 4

[Josh Aas]

(In reply to comment #3)
> > I think this patch has pretty high inherent risk.
> 
> Really?  My patch for bug 611910 has been on the trunk, the 1.9.1 branch and
> the 2.0 branch for quite a while without causing trouble.

However, the 1.9.2 patch is not the same and the difference is not trivial - it led to at least one very serious problem already.

> > How many crashes does this fix?
> 
> Not many.

Given that this patch doesn't fix many crashes and that it has caused serious problems in the past due to its modification of the tracking of core plugin objects, I don't think we should take it. I'm going to cancel the review request, I'm not giving it r- because I don't know that the patch itself isn't good - it very well might be.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Yes, my original 1.9.2-branch for bug 611910 caused a bad bug (bug
629030).  But the problem, once I discovered it, was very simple, and
so is the fix.

Just because my previous patch caused such a bad bug, though, we
should do more testing of this patch -- yet it appears nobody has
tested it besides myself.

And (as we've both noted) the patch for bug 611910 doesn't fix many
crashes.

So I'm fine with deep-sixing this patch ... though for somewhat
different reasons.