Closed Bug 1221448 Opened 9 years ago Closed 9 years ago

ESR 38.4 crashes with Java VM plugin enabled website [@ mozilla::plugins::parent::_releaseobject ]

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Version:
38 Branch
Platform:
All
Windows
Type:
defect
Priority:
Not set
Severity:
blocker

Tracking

(firefox42 affected, firefox43 wontfix, firefox44 verified, firefox45 verified, firefox-esr3842+ verified, b2g-v2.5 fixed, thunderbird_esr38 fixed)

Status:
VERIFIED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox42 --- affected
firefox43 --- wontfix
firefox44 --- verified
firefox45 --- verified
firefox-esr38 42+ verified
b2g-v2.5 --- fixed
thunderbird_esr38 --- fixed

People

(Reporter: rajeshvigtcs, Assigned: qdot, NeedInfo)

References

(
URL
)

Details

Crash Data

Attachments

(3 files, 2 obsolete files)

Applet crash on FF42 with new release.jpg
210.82 KB, image/jpeg
Details
Firefox 42.0b8 Crash Report - Report ID c3322f94-4812-4aa1-a9cf-f66452151112.htm
443.80 KB, text/html
Details
Patch 2 (v2) - WIP: Leak instead of crashing on off-main-thread NPAPI _releaseobject
1.15 KB, patch
jandem
: review+
jst
: review+
benjamin
: review+
Sylvestre
: approval-mozilla-aurora+
lizzard
: approval-mozilla-release-
Sylvestre
: approval-mozilla-esr38+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20151027170520

Steps to reproduce:

Firefox got updated yesterday to 38.4 and since then its crashing 


Actual results:

https://crash-stats.mozilla.com/report/index/bff4044c-1731-458d-8d00-ee3d32151104

Check crash report, its crashing everytime JVM plugin enabled web sit is opened
Crash report : https://crash-stats.mozilla.com/report/index/bff4044c-1731-458d-8d00-ee3d32151104

Plugin : Java(TM) Platform SE 7 U85 - 10.85.2.15
URL: https://crash-stats.mozilla.com/repor...
OS: Unspecified → Windows 7
Priority: -- → P2
Hardware: Unspecified → x86_64
[Tracking Requested - why for this release]:
Always crashes pages with Java applets.

That might be a regression from bug 1140616 (access restricted) / https://www.mozilla.org/en-US/security/advisories/mfsa2015-130/

Confirming on Firefox 38.4.0 (32 bit) on Windows 8.1 (64 bit) with Java(TM) Platform SE 8 U65 (32 bit).
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ mozilla::plugins::parent::_releaseobject ]
status-firefox42: --- → unaffected
status-firefox-esr38: --- → affected
tracking-firefox-esr38: --- → ?
Component: Untriaged → Plug-ins
Ever confirmed: true
OS: Windows 7 → Windows
Priority: P2 → --
Product: Firefox → Core
Hardware: x86_64 → All
Summary: ESR 38.4 crashes with Java VM plugin enabled website → ESR 38.4 crashes with Java VM plugin enabled website [@ mozilla::plugins::parent::_releaseobject ]
Confirming this on Windows 10, Firefox 38.4.0, Java SE8U65 - We have an internal application at my company that causes the browser to crash after Java plugin is utilized.   The crash appears to happen at Oracle's "Do I have Java?" test page as well as an easy way to reproduce the problem.
Blocks: CVE-2015-7196
Severity: critical → blocker
Jan, this is a critical bug that is affecting several esr38.4.0 end-users (see bug 1140616 where we uplifted the patch). Is there a fix that is ready for uplift to esr38? We may need to consider doing a dot release for this esr38.4.0 to address this issue. Thanks!
Flags: needinfo?(jdemooij)
See messages on the ESR mailing list.
(In reply to Ritu Kothari (:ritu) from comment #5)
> Jan, this is a critical bug that is affecting several esr38.4.0 end-users
> (see bug 1140616 where we uplifted the patch). Is there a fix that is ready
> for uplift to esr38? We may need to consider doing a dot release for this
> esr38.4.0 to address this issue. Thanks!

Our only options are backing out bug 1140616 (not ideal for obvious reasons) or waiting for Oracle to fix the Java plugin.

David, any news on that?
Flags: needinfo?(jdemooij) → needinfo?(david.dehaven)
Dan, Al: We may need your help in determining what the right fix for this issue might be. Given that this is a sec issue but still needs a fix from Oracle, a backout of this fix may not be acceptable. Right?
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
Benjamin, as you may already know this bug is a huge concern for esr end-users and it is unclear what a good fix would be. Is it time to consider backporting the fix that moves java plug-ins OOP to esr38 to address this? Are there any other options?
Flags: needinfo?(benjamin)
We could block Java completely? This is a Java bug, and I don't think the situation has changed any from last time: moving Java out of process on ESR would affect more people negatively than this bug.

Donald Smith from Oracle is already aware of the plugin bug.
Flags: needinfo?(benjamin)
End users use java applications are going to be forced to return to Firefox 38.3 ESR or 42 until the correction of this crash.

On Monday, we will deploy Firefox 38.3 ESR and prohibit updates.
We can not read the contents of the security bug concerned. Why it works with Firefox 42 and not with Firefox 38.4 ESR?
Oracle is aware of the issue in 38.4 and we believe we have a fix. 

Benjamin, depending on when we can make it available, would the "block Java completely" you mention mean blocking specific to 38.4 given everything works fine today in 42+ and 38.3?

Also, forgive my ignorance to the typical user-version profiles for Firefox, but can someone help me understand what % and profile of users would be locked into 38.4 without an easy path to 42+?
Flags: needinfo?(david.dehaven)
> Also, forgive my ignorance to the typical user-version profiles for Firefox, but can someone help me understand what % and profile of users would be locked into 38.4 without an easy path to 42+?

The Enterprise Support Release is staying at Firefox 38.X until Firefox 45. This version is used by enterprises (and is probably your primary consumer of Java in the browser).
This bug appears to have some relationship to Bug 1222229 (https://bugzilla.mozilla.org/show_bug.cgi?id=1222229). I have placed reproduction steps for that bug in its thread. We are able to successfully open an applet using 38.4 (ESR), but this bug (1221448) has a similar applet-shutdown crash, except instead of crashing just the plugin, Firefox itself crashes entirely when the applet (any applet) is shut down by navigating away from it or refreshing page, etc. From what I am reading here, the thought was that 1221448 only affects the ESR, but there appears to be a wider problem (or same problem) that affects multiple versions.
1 - open applet
2 - refresh page (crashes Firefox)
I only see one crash with this signature from ESR 38.4.0, is this really an ESR-only problem?
Flags: needinfo?(dveditz)
Crash reports get created for both 38.4 and 42.0 - for the latter one, the user might not notice it, e.g. the the check for Java on http://java.com/en/download/installed8.jsp successfully shows the version number and no crash UI on 42.0, but creates two crash reports.
status-firefox42: unaffected → affected
status-firefox43: --- → affected
status-firefox44: --- → affected
status-firefox45: --- → affected
See Also: → 1222036
Yes, this is not isolated to ESR 38.4 but can also be clearly seen on FF 42 (see bug 1222036). Our application and users are severely affected by this crash. Do we have an ETA on its resolution?
Flags: needinfo?(abillings)
Hi Donald, how soon can you guys make the fix available? As you can see from the comments on this bug and other related bugs, this seems to be severely impacting our end-users and we will really appreciate a quick resolution. Please let us know if we can help in anyway.
Flags: needinfo?(donald.smith)
We believe we have a fix, and are working to make it available publicly in the next 48-72h.  The facts related to FF 42 was new information to us and so it warranted further testing before pushing, until then this was believed to be isolated to 38.4.
Flags: needinfo?(donald.smith)
Hi Donald, request you to provide some more additional information to confirm. Will the fix be a part of new Java-plugin for FF? (or) that will be released as a patch for existing plug-in?. Can you please further explain on any further impacts due to the new fix that will be released?
Flags: needinfo?(donald.smith)
(In reply to Donald Smith from comment #24)
> We believe we have a fix, and are working to make it available publicly in
> the next 48-72h.  The facts related to FF 42 was new information to us and
> so it warranted further testing before pushing, until then this was believed
> to be isolated to 38.4.

This is great! I look forward to getting more details from you on how you plan to roll out the fix and I am hoping to let our Firefox end-users know so they get unblocked. Many thanks to you and your team for pushing hard on this one.
Quick update - we expect to have something up tomorrow mid day pacific that should address the issues, and after confirming more broadly the fix does what it should, we'll push more broadly.  I'll ping back as soon as it's live with the right links/info.  Thx.
Flags: needinfo?(donald.smith)
Are we also aiming at fixing the issue with FF42 reported on Bug 1222036 with this release? If not, then what's the plan on that one?
(In reply to Bina from comment #29)
> Are we also aiming at fixing the issue with FF42 reported on Bug 1222036
> with this release? If not, then what's the plan on that one?

Bina, as far as I understand, the same fix from Oracle (when released), will address issues in both ESR38.4.0 and FF42. Thanks!
Ritu, That is great - thanks for confirming!
You can now get version 1.8.0_66-b18, of the JRE or JDK, here:
http://www.oracle.com/technetwork/java/javase/downloads/jre8-downloads-2133155.html
http://www.oracle.com/technetwork/java/javase/downloads/jdk8-downloads-2133151.html

Note that you want b18, and you want 8u66, *not* 8u55.  Also this is Windows only.  Build 1.8.0_66-b18 contains the fix for this issue.  Be sure to check double check after install with "java -version", or after accepting the license you'll notice the url path for the download includes -b18 instead of -b17.  It is possible you may hit a cache with -b17 still out there.

We'd greatly appreciate your help testing this out.  Once we've had more public validation of this fix, we'll push more aggressively, likely very early next week.  If you experience any issues, please report at java.com/report

FWIW, release notes are here:
http://www.oracle.com/technetwork/java/javase/8u66-relnotes-2692847.html
I know that Java 7 is EOL, but do we know if this problem also affects Java 7?

The issue here is that there are a lot of people still running Java 7 (particularly enterprises) and if it affects 7, they're in a pickle.

Is it possible to simply rollback the releaseObject call and move back to the main thread - at least for the ESR version?  I'm guessing this was done as part of the e10s work, but I have no idea.

Also, why doesn't this affect Mac or Linux?
FWIW, Java 7 is not EOL, it's just no longer publicly updated.  Enterprises with needs for Java 7 should work through support for details on impact and availability of patches. 

That being said, it is quite possible to use current plugins with older versions of Java if necessary.  It may be a bit dated, but this article gives the gist -- https://blogs.oracle.com/java-platform-group/entry/managing_multiple_java_versions
For "my PCs" the new Java Version 8U66 solved the problem.
Looks like on FF42.0, the issue we experienced still occurs with the new release. See attached below and let me know if you need more info.
Tested with ESR 38.4.0 and it's working fine.

Thanks!
After test with new jvm 8U66, solved ESR 38.4.0, however not for FF42.0, still crash.
The result of our tests :

1) Windows 7, FF38.4 & jre 8u66 : it works
2) Windows XP, FF38.3 & jre 7u80 : it works
3) Windows XP, FF38.4 & jre 7u80 : it crashes...

Thank you for the Windows 7 patch.

Can we expect a solution for computers running Windows XP ? An unsupported platform by Oracle...
Actually, I don't think it's an XP problem.  I think Java 7 just has the same bug as Java 8.  As Donald mentioned above, my guess is Java 7 won't be fixed unless some corporate entity steps up to pay to fix it.  Even then, it may be that the paid for fix is only released to that company.  Donald can explain better.
Java 7 updates since last April are only available to supported Oracle customers through their usual support channels.  Publicly available and publicly supported versions of Java is currently Java 8.
FWIW, the policies, dates and version information for all this can be found here:

http://www.oracle.com/technetwork/java/eol-135779.html
No crashes or out of process crash reports in Firefox 38.4.0 (20151027170520) and 42.0 (20151029151421) - both 32 bit versions - on Windows 8.1 64 bit when tested on http://java.com/en/download/installed8.jsp

Bina and Jelm, please submit your crash reports and post crash IDs here. You can access them like described in https://support.mozilla.org/en-US/kb/mozillacrashreporter#w_viewing-crash-reports7

Thank you.
Flags: needinfo?(jelazaro)
Flags: needinfo?(btodi)
We've not been able to reproduce the issues reported here against FF42 with 8u66b-18.  If anyone has additional information or more information to help reproduce, can you please contact david.dehaven at Oracle.
Sebastian, 

My crash ID: c3322f94-4812-4aa1-a9cf-f66452151112
Crash report: attached below
Flags: needinfo?(btodi)
Bina,
This is not the same crash that we've just fixed with b18. Can you please file a new issue for this at bugs.java.com? It would be incredibly helpful if you could provide more information about the application, specifically what it was doing at the time it crashed (it looks like it was trying to use the file services). If you can narrow it down to a specific test case, please do so. You should get an incident number when you file, please forward that to me.
Hello.
We have the same problem. 
We tried java 8 u66, but situation didn't change.
Crash id 573c4602-eccf-4af5-8d3a-96ee82151113.
8u66 is not specific enough.  You must be absolutely sure you're using 8u66b18, available only on OTN.  Are you sure you were using 8u66b18?

Also, can you clarify which "situation" you refer to?  FF38.4, or FF42?
(In reply to Bina from comment #47)
> My crash ID: c3322f94-4812-4aa1-a9cf-f66452151112
> Crash report: attached below
Thank you for the report. This is different and seems to be new in java 8u66b.

(In reply to lenavl from comment #50)
> We have the same problem. 
> We tried java 8 u66, but situation didn't change.
> Crash id 573c4602-eccf-4af5-8d3a-96ee82151113.
Thank you for the report. That is a different issue, but has already 530 submitted crash reports in the last 7 days. I filed bug 1224549 for it.
We're seeing FF38.4 and FF42.0 working properly with 8u66b18 in Windows 7.
Java 8u66b18 fixed our problems with Firefox42 on Windows machines.
I'm wondering how this is a bug in Java and not Firefox.

I've had Firefox ESR 38.2.0 installed for some time as well as JRE 7u85.  They've been working fine, and I did confirm the JRE verify page did not crash Firefox.

As soon as I installed ESR 38.4.0, Java started crashing.

Like others, my enterprise is still on JRE 7, and for some time to come.

Did something change in Firefox from 38.2 to 38.4 that triggered a bug in Java, or did something change in Firefox that now crashes Java?
(In reply to Marc Meltzer from comment #55)
> Did something change in Firefox from 38.2 to 38.4 that triggered a bug in
> Java, or did something change in Firefox that now crashes Java?
See comment 9: It's because of the change in bug 1140616.
(In reply to Sebastian H. [:aryx][:archaeopteryx] from comment #56)
> (In reply to Marc Meltzer from comment #55)
> > Did something change in Firefox from 38.2 to 38.4 that triggered a bug in
> > Java, or did something change in Firefox that now crashes Java?
> See comment 9: It's because of the change in bug 1140616.

Also note that bug 1140616 forces a crash under these conditions, but the alternative is even worse -- NPAPI is a single threaded API, and a plugin (in this case Java) making calls off the main thread is undefined behaviour. That is the wrong thing to do, full stop. So the solution isn't to back out the forced crash, the solution is to fix the plugin.
> --- Comment #32 from Donald Smith <donald.smith@oracle.com> 2015-11-11 12:35:54 PST ---
>
> You can now get version 1.8.0_66-b18, of the JRE or JDK, here:
> http://www.oracle.com/technetwork/java/javase/downloads/jre8-downloads-2133155.html
> http://www.oracle.com/technetwork/java/javase/downloads/jdk8-downloads-2133151.html

When will this fix be a real release? Will it only be fixed in 1.8.0_66 or will there be a new version? We need to inform our customers.
Can you elaborate on what a real release is and what you would like to see? 

8u66 has been released on the Oracle website at the links Donald provided above. Additionally, as of Monday 12 noon Pacific Time, 8u66 was released on java.com.
When will the security bug be opened so we can see the fix?

Was there really no way to workaround/fix for the security bug except by making Firefox crash?

Maybe the thing to do here is (as bad as it sounds) is put this security fix behind a preference so that companies can make their own decisions (especially if they don't allow Java on the web, just inside their companies.)

The alternative is people are going to be stuck on the old ESR until they can roll out a new Java (which requires a lot more qualification than a browser).
(In reply to Mike Kaply [:mkaply] from comment #60)

You hit the nail directly on the head. Our company has customers in defense and aerospace that are using FF + Java and Java gets far more scrutiny than FF does, and for good reason. Based on posts above, there is a bit of arrogance in that the most 'pure' solution *must* be implemented, which ignores the real world impact utterly. I appreciate that Oracle had a mistake in the plugin, but blowing up the browser is not an acceptable solution.

> When will the security bug be opened so we can see the fix?
> 
> Was there really no way to workaround/fix for the security bug except by
> making Firefox crash?
> 
> Maybe the thing to do here is (as bad as it sounds) is put this security fix
> behind a preference so that companies can make their own decisions
> (especially if they don't allow Java on the web, just inside their
> companies.)
> 
> The alternative is people are going to be stuck on the old ESR until they
> can roll out a new Java (which requires a lot more qualification than a
> browser).
(In reply to Scott Munson from comment #61)
> (In reply to Mike Kaply [:mkaply] from comment #60)
> 
> You hit the nail directly on the head. Our company has customers in defense
> and aerospace that are using FF + Java and Java gets far more scrutiny than
> FF does, and for good reason. 

I would think that people working in the defense industry would prefer to have their browser crash when Java does something insecure and unsafe rather than potentially be exploited and possibly have malware installed on their machine.
(In reply to Al Billings [:abillings] from comment #62)
> 
> I would think that people working in the defense industry would prefer to
> have their browser crash when Java does something insecure and unsafe rather
> than potentially be exploited and possibly have malware installed on their
> machine.

Java is accessed on isolated networks, making such a case extremely unlikely, and updating software doubly difficult. IT security for the DoD updates browsers and Java when windows of time allow (e.g. in between missions) and some of them got stuck with this problem. Intermediate updates require special intervention to replace applications like Java. Side note - they do not use ESR (oddly).

The end user has no idea what has happened except that they "can't do Java", which is what we hear from them. They don't know if a crash is caused by the browser, Java, some security problem, etc. There must be alternatives that can equally protect against the problem beyond a full-up crash, and would be more informative as to what has happened.
Here is what I've found:

ESR 38.4 and JRE 7u91b32 installed.

Connect to http://java.com/en/download/installed.jsp and allow the Java applet to run.  It appears to run successfully without crashing the browser.  This page wants to uninstall my old version of the JRE, but I don't have admin rights.  At least it doesn't crash the browser.

If I connect to http://java.com/en/download/installed8.jsp (notice the '8' at the end of the URL), the browser crashes.

Here are two crash reports:
https://crash-stats.mozilla.com/report/index/bp-c3fd0eda-8454-451b-a3c1-bcad12151119
https://crash-stats.mozilla.com/report/index/bp-50d2ab70-5e18-487a-b50c-558c72151120

It looks like the first URL is a JRE 7 applet, so the installed JRE 7 behaves properly.  The second URL, however, probably has JRE 8 code somewhere in it, and since my JRE isn't 8, it crashes the browser.

Just a guess.

Is there anything else that [can/needs to] be fixed in Java 7 to resolve this ongoing issue?
BDS, should we consider blocking Java7 (as you had suggested earlier) as Oracle has no plans to roll out a fix similar to the one they did for Java 8? This request is mainly coming from the fact that our crash data shows that there is a huge spike in Firefox ESR38.4 crashes owing to java 7.
Flags: needinfo?(benjamin)
@Marc, you should work with Oracle support on plugin issues related to your use of Java 7.  I believe we have some fixes/workarounds/more info available there.  If there are issues related to interop between your use of 7 and 8, you should work with Oracle support on that.

@Ritu, the security baseline for Java 7 is 7u91.  It would be fine to block everything < 7u91.  But we do have support available to those enterprises using Java 7 still, and those folks should be working with Oracle on their plugin issues.  So a total block of 7 I don't think will help there.
> @Ritu, the security baseline for Java 7 is 7u91.  It would be fine to block
> everything < 7u91.  But we do have support available to those enterprises
> using Java 7 still, and those folks should be working with Oracle on their
> plugin issues.  So a total block of 7 I don't think will help there.

Hi Donald, thanks for the additional info. If you guys are getting enough requests from Firefox ESR end-users about needing fixes in Java7, would it be useful to make the fix available publicly? Mozilla could also share this link with any new/existing ESR users who continue to be troubled by the crashes.
Hi Ritu,

Java 7 should not be used by the general public, there haven't been publicly available security updates since April 2015.  Java 7 should only be used by customers under Oracle support, usually enterprise type users.  Ditto to Java 6, we still have enterprise users of Java 6, but it shouldn't be used by the public.  Our autoupdates to public users for well over a year now should be moving everyone to 8 (unless under Oracle support, and managed within an enterprise).

Java 8 is the current version of Java SE that the general public should be using.
We are blocking the insecure versions of Java 7, but I don't think we should issue a block just for the stability issues: Java is already click-to-play by default, and users who still have Java 7 probably aren't upgrading on purpose and the block won't change that.
Flags: needinfo?(benjamin)
Since I'm helping maintain NPAPI, I got asked to look into this to see if there's any possible solution on our side.

It looks like for NPObject*'s, we expect a _createobject/_releaseobject pair. If _createobject is called off the main thread, it returns a nullptr. _releaseobject used to just try to release memory no matter what thread it was called on, now it crashes as part of the security fix.

Is it possible that, since (I believe) we know any NPObject* will have be created on the main thread, we could forward non-main-thread _releaseobject calls back to the main thread for processing? It doesn't look like there's anything happening in _releaseobject that would need to be forwarded back to the thread that originated the call.
Flags: needinfo?(jdemooij)
Assignee: nobody → kyle
Ok, I talked to :jandem on IRC, and between that and realizing my last comment restates bz's take on this (https://bugzilla.mozilla.org/show_bug.cgi?id=1140616#c26), I'm trying the "forward to main thread" idea. This is a WIP patch. Now I've just got to figure out how to test it. I think I can possibly add a case to the nptest plugin code.

If anyone that was having crashes wants to install:

http://archive.mozilla.org/pub/firefox/try-builds/kmachulis@mozilla.com-ed1e6e90894a/try-win32/firefox-38.4.0esrpre.en-US.win32.installer.exe

And let me know if it works for them, that'd be great.
Kyle, fyi (and no pressure :p) but we are planning to build esr 38.5 with a fix for this bug next Monday. We can delay it by one or two days for this patch
I will test it today when I get a chance. The async release solution sounds fine to me, I don't see why it would cause any issues on our side.
I tested this with an older JRE that does not have our _releaseObject fix and it seems to work fine with this test build. I verified against FF42 and 38.4 ESR which do crash when running the same tests.
(In reply to Sylvestre Ledru [:sylvestre] from comment #72)
> Kyle, fyi (and no pressure :p) but we are planning to build esr 38.5 with a
> fix for this bug next Monday. We can delay it by one or two days for this
> patch

Well, I'll put it in for review now, then try to get the tests in I guess.
Comment on attachment 8695615 [details] [diff] [review]
Patch 1 (v1) - WIP: Make NPAPI forward non-main-thread _releaseobject calls to the main thread.

Asking for reviews from :jandem (original patch dev) and bz (since we had basically the same idea and I want to make sure it lines up).

bz: If you're too busy, feel free to unassign. Just trying to get backup reviews since this apparently needs to land quickly.
Attachment #8695615 - Flags: review?(jdemooij)
Attachment #8695615 - Flags: review?(bzbarsky)
Comment on attachment 8695615 [details] [diff] [review]
Patch 1 (v1) - WIP: Make NPAPI forward non-main-thread _releaseobject calls to the main thread.

r=me, though this gives me the willies.  Of course everything about this code gives me the willies...
Attachment #8695615 - Flags: review?(bzbarsky) → review+
Comment on attachment 8695615 [details] [diff] [review]
Patch 1 (v1) - WIP: Make NPAPI forward non-main-thread _releaseobject calls to the main thread.

This is not safe. NPObjects are destroyed when their plugin instance is town down, and so blindly proxying the release without tracking the destroy won't work. You could maybe hook into nsJSNPRuntime::OnPluginDestroy to cancel pending releases of objects that are destroyed.
Attachment #8695615 - Flags: review?(bzbarsky)
Attachment #8695615 - Flags: review-
Attachment #8695615 - Flags: review+
Attachment #8695615 - Flags: review?(bzbarsky)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #78)
> Comment on attachment 8695615 [details] [diff] [review]
> Patch 1 (v1) - WIP: Make NPAPI forward non-main-thread _releaseobject calls
> to the main thread.
> 
> This is not safe. NPObjects are destroyed when their plugin instance is town
> down, and so blindly proxying the release without tracking the destroy won't
> work. You could maybe hook into nsJSNPRuntime::OnPluginDestroy to cancel
> pending releases of objects that are destroyed.

I'm not sure I follow here (my understanding of the system here is still fairly cursory). Are you saying I should set mDestroyPending to false while iterating through objects in OnPluginDestroy?
Flags: needinfo?(jdemooij) → needinfo?(benjamin)
Comment on attachment 8695615 [details] [diff] [review]
Patch 1 (v1) - WIP: Make NPAPI forward non-main-thread _releaseobject calls to the main thread.

Review of attachment 8695615 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this.

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #79)
> I'm not sure I follow here (my understanding of the system here is still
> fairly cursory). Are you saying I should set mDestroyPending to false while
> iterating through objects in OnPluginDestroy?

IIUC we're worried about this scenario:

(1) We dispatch ReleaseObjectRunnable for a certain NPObject.
(2) Before ReleaseObjectRunnable can run, we destroy the plugin, including our NPObject.
(3) ReleaseObjectRunnable runs and does bad things, because x has already been destroyed.

::: dom/plugins/base/nsNPAPIPlugin.cpp
@@ +1410,5 @@
>  
> +class ReleaseObjectRunnable : public nsRunnable
> +{
> +public:
> +  ReleaseObjectRunnable(NPObject* aObject) :

Nit: make this |explicit| to avoid static analysis bustage.
Attachment #8695615 - Flags: review?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #80)
> Comment on attachment 8695615 [details] [diff] [review]
> Patch 1 (v1) - WIP: Make NPAPI forward non-main-thread _releaseobject calls
> to the main thread.
> 
> Review of attachment 8695615 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for working on this.
> 
> (In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment
> #79)
> > I'm not sure I follow here (my understanding of the system here is still
> > fairly cursory). Are you saying I should set mDestroyPending to false while
> > iterating through objects in OnPluginDestroy?
> 
> IIUC we're worried about this scenario:
> 
> (1) We dispatch ReleaseObjectRunnable for a certain NPObject.
> (2) Before ReleaseObjectRunnable can run, we destroy the plugin, including
> our NPObject.
> (3) ReleaseObjectRunnable runs and does bad things, because x has already
> been destroyed.

This is as much a question for bsmedberg as it is for you, mainly just because I'm having problems tracing this code too...

How would we be destroying our NPObject before we release it? That's why I forwarded and exited early in _releaseobject, because it looked like after most called to _releaseobject we just assumed things were dead anyways?
For ESR 38 only, we could leak to fix older Java 7 crashes for now. This is not a permanent solution by any means, but buys us a bit more time to work on one.

I honestly have no idea how this will act or how much memory we'll lose. But it's an option.
Attachment #8696525 - Flags: review?(jst)
Attachment #8696525 - Flags: review?(jdemooij)
Attachment #8696525 - Flags: review?(benjamin)
Uploaded wrong diff
Attachment #8696525 - Attachment is obsolete: true
Attachment #8696525 - Flags: review?(jst)
Attachment #8696525 - Flags: review?(jdemooij)
Attachment #8696525 - Flags: review?(benjamin)
Attachment #8696526 - Flags: review?(jst)
Attachment #8696526 - Flags: review?(jdemooij)
Attachment #8696526 - Flags: review?(benjamin)
Comment on attachment 8696526 [details] [diff] [review]
Patch 2 (v2) - WIP: Leak instead of crashing on off-main-thread NPAPI _releaseobject

Review of attachment 8696526 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM as a stopgap, though I'm not really familiar with NPAPI (other than adding the offending MOZ_CRASH).
Attachment #8696526 - Flags: review?(jdemooij) → review+
(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #81)
> How would we be destroying our NPObject before we release it? That's why I
> forwarded and exited early in _releaseobject, because it looked like after
> most called to _releaseobject we just assumed things were dead anyways?

He's talking about the situation where one or more NPObjects are scheduled to be released (by forwarding to the main thread) prior to applet destruction then the applet gets destroyed before the scheduled release tasks are processed, as it's written you could end up with a double free. It's a common concurrency issue, especially when dealing with object lifecycles. You just need synchronization somewhere. Maybe a queue? Possibly some way to identify applets post-mortem? If I knew more about the code I could make valid suggestions :)
Kyle, I know the timing is not perfect but any chance we could have a fix today? I am waiting for this to build ESR 38.5. Thanks
Flags: needinfo?(kyle)
(In reply to Sylvestre Ledru [:sylvestre] from comment #86)
> Kyle, I know the timing is not perfect but any chance we could have a fix
> today? I am waiting for this to build ESR 38.5. Thanks

Not sure how I'm supposed to do that if I can't get reviews. I'm not at the work week so I can't poke people in person either.
Flags: needinfo?(kyle)
Comment on attachment 8696526 [details] [diff] [review]
Patch 2 (v2) - WIP: Leak instead of crashing on off-main-thread NPAPI _releaseobject

Sorry for the delay. r=jst
Attachment #8696526 - Flags: review?(jst) → review+
Kyle, I think we can land that now that we have two reviewers. Could you fill an uplift request to esr38, release (even if it is probably too late) and aurora?
Many thanks

NI Liz for your information :)
Flags: needinfo?(lhenry)
Flags: needinfo?(kyle)
Keywords: checkin-needed
Comment on attachment 8696526 [details] [diff] [review]
Patch 2 (v2) - WIP: Leak instead of crashing on off-main-thread NPAPI _releaseobject

I guess we can't add telemetry for this since plugin processes don't do telemetry. This will only leak until this instance is torn down, which might be ok.

It's probably worth doing the same thing in http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginModuleChild.cpp#2308 for IPC plugins.
Flags: needinfo?(benjamin)
Attachment #8696526 - Flags: review?(benjamin) → review+
Attachment #8695615 - Attachment is obsolete: true
Flags: needinfo?(kyle)
Comment on attachment 8696526 [details] [diff] [review]
Patch 2 (v2) - WIP: Leak instead of crashing on off-main-thread NPAPI _releaseobject

Approval Request Comment
[Feature/regressing bug #]: Bug 1140616
[User impact if declined]: Java Applets using publicly available versions of Java 7 crash
[Describe test coverage new/current, TreeHerder]: No tests as of yet, but we're on a short timeline.
[Risks and why]: Leaks memory, but we at least still warn
[String/UUID change made/needed]: None
Attachment #8696526 - Flags: approval-mozilla-release?
Attachment #8696526 - Flags: approval-mozilla-esr38?
Attachment #8696526 - Flags: approval-mozilla-aurora?
Comment on attachment 8696526 [details] [diff] [review]
Patch 2 (v2) - WIP: Leak instead of crashing on off-main-thread NPAPI _releaseobject

Taking it to fix this crash, many thanks
Attachment #8696526 - Flags: approval-mozilla-esr38?
Attachment #8696526 - Flags: approval-mozilla-esr38+
Attachment #8696526 - Flags: approval-mozilla-aurora?
Attachment #8696526 - Flags: approval-mozilla-aurora+
Tomcat, could you land that? Thanks
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/94f4cfa98356
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8696526 [details] [diff] [review]
Patch 2 (v2) - WIP: Leak instead of crashing on off-main-thread NPAPI _releaseobject

Too last minute for 43, but I'm glad this made it to 44.
Flags: needinfo?(lhenry)
Attachment #8696526 - Flags: approval-mozilla-release? → approval-mozilla-release-
QA Contact: cornel.ionce
I've been able to reproduce this issue on Firefox 38.4.0 ESR (build ID: 20151027170520)

Report IDs:
bp-33ccae6e-52f8-4927-a55f-b30f42151211
bp-c8034374-0e9a-4099-8ebf-b67632151211


Confirming the fix on:
*Firefox 38.5.0 ESR (build ID: 20151210074901)
*Firefox 45.0a1 Nightly
*Firefox 44.0a2 Aurora.

Testing was performed on Windows 7x64.
Status: RESOLVED → VERIFIED
status-firefox44: fixed → verified
status-firefox45: fixed → verified
status-firefox-esr38: fixed → verified
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: