Closed Bug 1013972 Opened 6 years ago Closed 6 years ago

crash in nsJSNPRuntime::OnPluginDestroy(_NPP*) with address 0x5a5a5a66, related to Java

Categories

(Core :: Plug-ins, defect, critical)

26 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox30 + verified
firefox31 + verified
firefox32 + verified

People

(Reporter: kairo, Assigned: terrence)

References

Details

(Keywords: crash, topcrash-win, verifyme)

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-059eeb35-64c3-42fb-adf9-dae6d2140516.
=============================================================

This is one of the main regressions between 29 beta and 30 beta, currently sitting at #7 on 30.0b, all Windows versions are affected.
It's a browser crash, and according to a comment, may happen at Firefox shutdown, some URLs visible are (there are a number of internal-looking ones as well, I guess a large number of pages with applets is probably affected):

32 	https://ebanking.bankmellat.ir/ebanking/fundsTransfer.bm
29 	https://muisca.dian.gov.co/WebCargamasivaexogena/DefCargaArchivoExo.faces
18 	https://www.e-cnhsp.sp.gov.br/

The crash is not new in 30, but seems to have risen significantly in volume with 30 beta.

Top frames:
0 	xul.dll 	nsJSNPRuntime::OnPluginDestroy(_NPP *) 	dom/plugins/base/nsJSNPRuntime.cpp
1 	xul.dll 	nsNPAPIPluginInstance::Stop() 	dom/plugins/base/nsNPAPIPluginInstance.cpp
2 	xul.dll 	nsPluginHost::StopPluginInstance(nsNPAPIPluginInstance *) 	dom/plugins/base/nsPluginHost.cpp
3 	xul.dll 	nsObjectLoadingContent::DoStopPlugin(nsPluginInstanceOwner *,bool,bool) 	content/base/src/nsObjectLoadingContent.cpp
4 	xul.dll 	nsObjectLoadingContent::StopPluginInstance() 	content/base/src/nsObjectLoadingContent.cpp
5 	xul.dll 	nsObjectLoadingContent::UnloadObject(bool) 	content/base/src/nsObjectLoadingContent.cpp
6 	xul.dll 	CheckPluginStopEvent::Run() 	content/base/src/nsObjectLoadingContent.cpp
7 	xul.dll 	nsBaseAppShell::RunSyncSectionsInternal(bool,unsigned int) 	widget/xpwidgets/nsBaseAppShell.cpp
8 	xul.dll 	nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal *,bool,unsigned int) 	widget/xpwidgets/nsBaseAppShell.cpp
9 	xul.dll 	nsThread::ProcessNextEvent(bool,bool *) 	xpcom/threads/nsThread.cpp
10 	xul.dll 	NS_ProcessNextEvent(nsIThread *,bool) 	xpcom/glue/nsThreadUtils.cpp
[...]

More reports at https://crash-stats.mozilla.com/report/list?signature=nsJSNPRuntime%3A%3AOnPluginDestroy%28_NPP%2A%29

Benjamin, who should look into this?
Flags: needinfo?(benjamin)
johns unless he doesn't have time, or then me :-(

Likely a regression from bug 975098, although that was actually supposed to prevent something which looks very similar.
Assignee: nobody → jschoenick
Blocks: 975098
Flags: needinfo?(benjamin)
This smells more like Bug 965830 or Bug 959787, since we have a free'd npobj in the nsJSNPRuntime hash.

@jonco - Can you see any obvious way we would have a free'd npobj here?

http://hg.mozilla.org/releases/mozilla-beta/annotate/25886b573924/dom/plugins/base/nsJSNPRuntime.cpp#l1865
Flags: needinfo?(jcoppeard)
(In reply to John Schoenick [:johns] from comment #4)
It's possibly related to the changes in bug 934421.  I'm investigating.
Flags: needinfo?(jcoppeard)
This crash continues to rise and is currently #12 in Firefox 31 (up 7 positions since last week).
Attached patch plugin-wrapper-assertion (obsolete) — Splinter Review
I looked but I can't see how this could happen.

One thing we should do however is to check that the wrapper really is removed from the hash table before it is freed.

This patch adds a release mode assert for this.  Hopefully this might help pinpoint what's going wrong.
Attachment #8427080 - Flags: review?(jschoenick)
Keywords: leave-open
Is js::HashMap::Enum reentrant-safe? The _releaseobject at http://hg.mozilla.org/mozilla-central/annotate/48ef1c52dece/dom/plugins/base/nsJSNPRuntime.cpp#l1858 can/does re-enter the Java plugin, which has been known to release other related NPObjects which are in the hash.

This is why the OOPP version of this code neuters the hashtable before enumerating it, see http://hg.mozilla.org/mozilla-central/annotate/48ef1c52dece/dom/plugins/ipc/PluginModuleChild.cpp#l2148 and bug 544074.
js/public/HashTable.h says "HashMap is not reentrant", so it sounds like no.
Right, I didn't realise these things could go reentrant, so my changes in bug 934421 are at fault here.

Here's a patch to address this.  This adds a flag to do the job of neutering the hashtable that the original code does.

Try build here: https://tbpl.mozilla.org/?tree=Try&rev=7542b26ff142
Assignee: jschoenick → jcoppeard
Attachment #8427080 - Attachment is obsolete: true
Attachment #8427080 - Flags: review?(jschoenick)
Attachment #8427830 - Flags: review?(benjamin)
Attachment #8427830 - Flags: review?(benjamin) → review+
Blocks: 934421
No longer blocks: 975098
Attachment #8427830 - Flags: checkin+
Attachment #8427830 - Flags: checkin+
Actually it's probably best to leave this open until we see whether the crashes stop.
Keywords: leave-open
(In reply to Jon Coppeard (:jonco) from comment #12)
> Actually it's probably best to leave this open until we see whether the
> crashes stop.

We only have a maximum of a handful crashes per day on nightly for this (and none so far in the 6 hours that the new nightly has been out), we really need to get this into tomorrow's beta to know if the crashes stop.
Comment on attachment 8427830 [details] [diff] [review]
bug1013972-reentrant-plugins

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Current #7 topcrasher.
User impact if declined: Crashes when using flash.
Testing completed (on m-c, etc.): 1-day on m-c: crashes gone.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.
Attachment #8427830 - Flags: approval-mozilla-beta?
As a note, I still do not see any crashes with the signature of this or bug 883134 in Nightly builds after this landed (i.e. 5/28 or newer). With the low volume on Nightly, this doesn't have to mean we're golden, but it surely doesn't look bad. ;-)
Comment on attachment 8427830 [details] [diff] [review]
bug1013972-reentrant-plugins

[Triage Comment]
We need this on Aurora too - no branch skipping.
Attachment #8427830 - Flags: approval-mozilla-release+
Attachment #8427830 - Flags: approval-mozilla-beta?
Attachment #8427830 - Flags: approval-mozilla-beta+
Comment on attachment 8427830 [details] [diff] [review]
bug1013972-reentrant-plugins

...and that requires Aurora approval (not release).
Attachment #8427830 - Flags: approval-mozilla-release+ → approval-mozilla-aurora+
This is a minimal, tailored backport of bug 993413 that should stop the crashes being experienced on beta that are caused by this bug.
Assignee: jcoppeard → terrence
Status: NEW → ASSIGNED
Attachment #8431000 - Flags: review+
Keywords: checkin-needed
The important question is whether this fixed most of the crashes in the last beta. If so, we should mark this bug FIXED and maybe have a followup for the small tail.
(In reply to Paul Silaghi, QA [:pauly] from comment #23)
> I see couple of reports on post-fix builds
> [...]
> Thoughts?

If you include the release channel in the display, you see that those are on the UX branch, where it might not have merged at the same time:
https://crash-stats.mozilla.com/search/?signature=~nsJSNPRuntime::OnPluginDestroy(_NPP*)&build_id=>20140527030202&version=32.0a1&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=release_channel

It sounds a lot to me like that one build on the UX branch didn't have it but starting with the next build, even that branch is fixed.

From what I saw, it also looks good on 30.0b9, I'm marking fixed for that based on Benjamin's comment and letting Paul or someone confirm and mark it verified. :)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Thanks Robert.
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #25)
> Paul or someone confirm and mark it verified. :)
It's a little early, let's wait a few more days.
verified on beta. the are no reports of this crash on Fx30b9 (Build ID: 20140529161749)
Keywords: leave-open
Target Milestone: --- → mozilla32
There still aren't any crash reports with this signature after the 20140528004008 build. I think that's enough to verify the fix.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.