Closed Bug 1013972 Opened 6 years ago Closed 6 years ago
crash in ns
JSNPRuntime::On Plugin Destroy(_NPP*) with address 0x5a5a5a66, related to Java
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?
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.
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
(In reply to John Schoenick [:johns] from comment #4) It's possibly related to the changes in bug 934421. I'm investigating.
This crash continues to rise and is currently #12 in Firefox 31 (up 7 positions since last week).
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.
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
Actually it's probably best to leave this open until we see whether the crashes stop.
(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.
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.
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+
I see couple of reports on post-fix builds https://crash-stats.mozilla.com/search/?signature=~nsJSNPRuntime%3A%3AOnPluginDestroy(_NPP*)&build_id=%3E20140527030202&version=32.0a1&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform Thoughts?
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 (:email@example.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)
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.
You need to log in before you can comment on or make changes to this bug.