Closed Bug 535298 Opened 16 years ago Closed 16 years ago

[OOPP] Loading https://www.mozilla.com/en-US/plugincheck/ crashes the browser [@ Abort ]

Categories

(Core Graveyard :: Plug-ins, defect)

Product:
Core Graveyard
Component:
Plug-ins
Platform:
x86_64
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(status1.9.2 .4-fixed)

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
status1.9.2 --- .4-fixed

People

(Reporter: u88484, Assigned: benjamin)

References

(
URL
)

Details

(4 keywords, Whiteboard: [fixed-lorentz])

Crash Data

Attachments

(4 files, 2 obsolete files)

Cancel the NotifyMaybeChannelError if the AsyncChannel is destroyed, rev. 1
3.00 KB, patch
cjones
: review+
Details | Diff | Splinter Review
Test for handling of crashes during NP_Destroy (and NPP_New), rev. 1.2
9.88 KB, patch
Details | Diff | Splinter Review
deterministic IPDL test for the known crashes
11.87 KB, patch
Details | Diff | Splinter Review
Work around async instantiation of plugins
4.24 KB, patch
Details | Diff | Splinter Review
Loading https://www.mozilla.com/en-US/plugincheck/ crashes the entire browser, no crash reporter. Works with 'dom.ipc.plugins.enabled;false'
Component: IPC → Plug-ins
QA Contact: ipc → plugins
Using today's second nightly, still crashes but crash reporter does catch the crash now. http://crash-stats.mozilla.com/report/index/d540bbb4-93d4-4a95-a152-1f4c92091216 Hmm, I guessing the [@ Abort ] isn't right since I got the same thing in bug 535280 but all of that stuff is above my head.
Keywords: crashreportid
Summary: [OOPP] Loading https://www.mozilla.com/en-US/plugincheck/ crashes the browser → [OOPP] Loading https://www.mozilla.com/en-US/plugincheck/ crashes the browser [@ Abort ]
Signature Abort UUID d540bbb4-93d4-4a95-a152-1f4c92091216 Time 2009-12-16 22:29:59.143556 Uptime 448 Last Crash 451 seconds before submission Product Firefox Version 3.7a1pre Build ID 20091216100208 Branch 1.9.3 OS Windows NT OS Version 6.1.7600 CPU x86 CPU Info AuthenticAMD family 15 model 104 stepping 2 Crash Reason EXCEPTION_ACCESS_VIOLATION Crash Address 0x0 User Comments Processor Notes Related Bugs Crashing Thread Frame Module Signature [Expand] Source 0 xul.dll Abort xpcom/base/nsDebugImpl.cpp:376 1 @0xbdd0707 2 xul.dll mozilla::ipc::AsyncChannel::Close ipc/glue/AsyncChannel.cpp:135 3 xul.dll mozilla::plugins::PluginModuleParent::NP_Shutdown dom/plugins/PluginModuleParent.cpp:573 4 xul.dll nsNPAPIPlugin::Shutdown modules/plugin/base/src/nsNPAPIPlugin.cpp:611 5 xul.dll nsPluginTag::TryUnloadPlugin modules/plugin/base/src/nsPluginTags.cpp:540 kurt: if you don't trust the stacks, please try windbg.
Filed bug 533548 on skipping `Abort` in crash-stats, since the important signature here is mozilla::ipc::AsyncChannel::Close. Chris, I think we probably need to be more lenient about double-closing the channel somehow, or provide an API that NP_Shutdown can call to see if it is already closed.
(In reply to comment #4) > Filed bug 533548 on skipping `Abort` in crash-stats Correct bug is bug 535548 for anyone curious.
(In reply to comment #0) > Loading https://www.mozilla.com/en-US/plugincheck/ crashes the entire browser, > no crash reporter. > > Works with 'dom.ipc.plugins.enabled;false' Hello Kurt --- could quickly clarify whether you see this OOPP enabled/disabled/both? The stack from comment 3 could only appear with OOPP enabled.
(In reply to comment #4) > Chris, I think we probably need to be more lenient about double-closing the > channel somehow, or provide an API that NP_Shutdown can call to see if it is > already closed. If we were to add that kind of hack, my vote is for an |if (mAlreadyShutdown) return;| early exit in PluginModuleParent::NP_Shutdown. Although that feels to me like admitting defeat; it appears that nsNPAPI* has shutdown behaviors that I don't fully understand yet. This Abort() could be caused by nsNPAPI* re-entering NP_Shutdown() on a crash notification, a la the hypothesis of bug 535321. This is worth having a unit test for.
(In reply to comment #6) > Hello Kurt --- could quickly clarify whether you see this OOPP > enabled/disabled/both? The stack from comment 3 could only appear with OOPP > enabled. No crash with OOPP disabled, page loads correctly and also correctly scans and displays results. Aforementioned crash with OOPP enabled.
I have a testcase for this: I don't think the plugin host is doing anything wrong here.
This patch creates a test that crashes during NPP_Destroy and from there aborts in NP_Shutdown because the channel is already closed.
Agreed this is a bug to fix, but do you know that that's what's causing this Abort()? (FWIW I can't repro on linux because plugincheck doesn't actually need to load the plugins.)
My test causes the abort by crashing the child within NPP_Destroy. #0 RealBreak () at ../../../src/xpcom/base/nsDebugImpl.cpp:421 #1 0x00007f5103d5ebf7 in NS_DebugBreak_P (aSeverity=3, aStr=0x7f5104285730 "Close() called on closed channel!", aExpr=0x0, aFile=0x7f5104285370 "../../../src/ipc/glue/AsyncChannel.cpp", aLine=135) at ../../../src/xpcom/base/nsDebugImpl.cpp:324 #2 0x00007f5103bdb691 in mozilla::ipc::AsyncChannel::Close (this=0x7f50f101c410) at ../../../src/ipc/glue/AsyncChannel.cpp:135 #3 0x00007f5103be7847 in mozilla::plugins::PPluginModuleParent::Close (this=0x7f50f101c400) at PPluginModuleParent.cpp:54 #4 0x00007f5103bcdf88 in mozilla::plugins::PluginModuleParent::NP_Shutdown (this=0x7f50f101c400, error=0x7fff0cbd12fe) at ../../../src/dom/plugins/PluginModuleParent.cpp:581 #5 0x00007f51038ef99b in nsNPAPIPlugin::Shutdown (this=0x7f50f4fc5200) at ../../../../../src/modules/plugin/base/src/nsNPAPIPlugin.cpp:621 #6 0x00007f5103918bb1 in nsPluginTag::TryUnloadPlugin (this=0x7f50fb19fd40) at ../../../../../src/modules/plugin/base/src/nsPluginTags.cpp:540 #7 0x00007f5103907b40 in nsPluginHost::ReloadPlugins (this=0x7f50f26fbca0, reloadPages=0) at ../../../../../src/modules/plugin/base/src/nsPluginHost.cpp:1867 #8 0x00007f5103906cf7 in nsPluginHost::SetUpPluginInstance (this=0x7f50f26fbca0, aMimeType=0x7f50f1289768 "application/x-test", aURL=0x7f50f10e5200, aOwner=0x7f50f16c0d40) at ../../../../../src/modules/plugin/base/src/nsPluginHost.cpp:2667 #9 0x00007f510390bb78 in nsPluginHost::InstantiateEmbeddedPlugin (this=0x7f50f26fbca0, aMimeType=0x7f50f1289768 "application/x-test", aURL=0x7f50f10e5200, aOwner=0x7f50f16c0d40) at ../../../../../src/modules/plugin/base/src/nsPluginHost.cpp:2417 #10 0x00007f5102e46cd9 in nsObjectFrame::InstantiatePlugin (this=0x7f50f2692328, aPluginHost=0x7f50f26fbca0, aMimeType=0x7f50f1289768 "application/x-test", aURI=0x7f50f10e5200) at ../../../src/layout/generic/nsObjectFrame.cpp:950 #11 0x00007f5102e4d3fa in nsObjectFrame::Instantiate (this=0x7f50f2692328, aMimeType=0x7f50f1289768 "application/x-test", aURI=0x7f50f10e5200) at ../../../src/layout/generic/nsObjectFrame.cpp:2047 #12 0x00007f51030a7683 in nsObjectLoadingContent::Instantiate (this=0x7f50f121d780, aFrame=0x7f50f2692370, aMIMEType=@0x7f50f111ee50, aURI=0x7f50f10e5200) at ../../../../src/content/base/src/nsObjectLoadingContent.cpp:1788 #13 0x00007f51030a8086 in nsAsyncInstantiateEvent::Run (this=0x7f50f111ee20) at ../../../../src/content/base/src/nsObjectLoadingContent.cpp:156 #14 0x00007f5103d4f674 in nsThread::ProcessNextEvent (this=0x7f510223a700, mayWait=0, result=0x7fff0cbd1c4c) at ../../../src/xpcom/threads/nsThread.cpp:527 ... Crashing in the implementation of NPP_Destroy immediately sets AsyncChannel::mCahnelState to mozilla::ipc::AsyncChannel::ChannelError, but the ActorDestroy sequence happens asynchronously and so we won't receive that notification until later.
I get make[6]: *** No rule to make target `test_crashing2.html'. Stop. make[6]: *** Waiting for unfinished jobs.... make[5]: *** [libs] Error 2 make[4]: *** [libs] Error 2 make[3]: *** [libs_tier_gecko] Error 2 make[2]: *** [tier_gecko] Error 2 make[1]: *** [default] Error 2 make: *** [build] Error 2 trying to apply the test patch on top of latest e10s.
Updated test with the files added and .reload() called
Attachment #418218 - Attachment is obsolete: true
JavaScript error: http://localhost:8888/tests/modules/plugin/test/test_crashing2.html, line 10: SimpleTest is not defined ++DOMWINDOW == 18 (0x7f1bef216c58) [serial = 64] [outer = 0x7f1bef215000] JavaScript error: http://localhost:8888/tests/modules/plugin/test/test_crashing2.html, line 15: A script from "http://localhost:8888" was denied UniversalXPConnect privileges. How do I make that go away?
Hmm, odd. If I run the test in a client browser (with another as a server), I get this error. But if I run in the server browser, I don't.
This patch doesn't fix all the errors in the testcase, but it does fix the ones I've diagnosed so far: * When NP_Shutdown is called on a plugin that has crashed (but ActorDestroy has not yet been processed), notice this in the channel and ignore it. * After the runnable for AsyncChannel::NotifyMaybeChannelError is sent but before we get to it in the event loop, we end up calling ~PluginModuleParent which destroys the AsyncChannel. Cancel the runnable if we end up in that situation. This still has a crash: IPC::Channel::ChannelImpl::set_listener(IPC::Channel::Listener*) (/builds/mozilla-central/ff-debug/dist/bin/libxul.so) IPC::Channel::set_listener(IPC::Channel::Listener*) (/builds/mozilla-central/ff-debug/ipc/chromium/../../../src/ipc/chromium/src/chrome/common/ipc_channel_posix.cc:788) mozilla::ipc::AsyncChannel::Clear() (/builds/mozilla-central/ff-debug/ipc/glue/../../../src/ipc/glue/AsyncChannel.cpp:300) ~AsyncChannel (/builds/mozilla-central/ff-debug/ipc/glue/../../../src/ipc/glue/AsyncChannel.cpp:77) ~SyncChannel (/builds/mozilla-central/ff-debug/ipc/glue/../../../src/ipc/glue/SyncChannel.cpp:70) ~RPCChannel (/builds/mozilla-central/ff-debug/ipc/glue/../../../src/ipc/glue/RPCChannel.cpp:81) ~PPluginModuleParent (/builds/mozilla-central/ff-debug/ipc/ipdl/PPluginModuleParent.cpp:39) ~PluginModuleParent (/builds/mozilla-central/ff-debug/dom/plugins/../../../src/dom/plugins/PluginModuleParent.cpp:94) ~nsNPAPIPlugin (/builds/mozilla-central/ff-debug/modules/plugin/base/src/../../../../../src/modules/plugin/base/src/nsNPAPIPlugin.cpp:291) nsNPAPIPlugin::Release() (/builds/mozilla-central/ff-debug/modules/plugin/base/src/../../../../../src/modules/plugin/base/src/nsNPAPIPlugin.cpp:221) I'm pretty sure that the IPC::Channel is already destroyed by the time we get to mozilla::ipc::AsyncChannel::Clear, and we're calling set_listener on bad memory, but I don't know exactly what the sequence is/should be.
Attachment #418259 - Flags: review?(jones.chris.g)
And now I can't reproduce the last crash I was talking about, so it may be much more random.
Attachment #418257 - Attachment is obsolete: true
cjones: Mochitest sets a bunch of prefs in the testing profile to give privileges to certain hosts. http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#284
Attachment #418259 - Flags: review?(jones.chris.g) → review+
Comment on attachment 418259 [details] [diff] [review] Cancel the NotifyMaybeChannelError if the AsyncChannel is destroyed, rev. 1 Came to almost the exact same solution when debugging the IPDL test ;).
Pushed http://hg.mozilla.org/projects/electrolysis/rev/74ff56f032bc Pushed http://hg.mozilla.org/projects/electrolysis/rev/07c66d63ecb7 Not sure what we want to do with the NPAPI tests, and they might trigger the JIT debugger on windows, so I'll leave this bug open until that's sorted.
I'm also hitting this crash a lot with GMail. Looks like this is at the top of the crash list at the moment too.
You may be experiencing a different crash. [@ Abort ] is too generic of a signature, bsmedberg filed bug 535548 on splitting it up in Socorro.
It's possible. The signature isn't exactly unique. If there is anything I can do to help you guys pinpoint this one let me know. It is IPC related however since it does not occur with ipc.plugins disabled.
(In reply to comment #28) > It's possible. The signature isn't exactly unique. If there is anything I can > do to help you guys pinpoint this one let me know. > > It is IPC related however since it does not occur with ipc.plugins disabled. File a new bug under Core:Plugins and please include the crashid from about:crashes.
Was waiting on the mochitests to be sorted before closing this.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
The test was buggy because of asynchronous instantiation, and I fixed it: http://hg.mozilla.org/mozilla-central/rev/43061853e246 And filed bug 536443 to find a better way to do that.
Status: RESOLVED → VERIFIED
Blanket approval for Lorentz merge to mozilla-1.9.2 a=beltzner for 1.9.2.4 - please make sure to mark status1.9.2:.4-fixed
(In reply to comment #35) > Merged into 1.9.2 at > http://hg.mozilla.org/releases/mozilla-1.9.2/rev/84ba4d805430 Something looks weird here. I'm not seeing any of the test_crashing*.html files on 1.9.2...
Did the tests not get included in the merge for 1.9.2?
They did. I copied the trunk contents of modules/plugin/test to the branch wholesale, rather than porting individual tests.
You're right. I see them in the mochitest log on 1.9.2. Marking this as verified for 1.9.2 since they are passing.
Keywords: verified1.9.2
Assignee: nobody → benjamin
Crash Signature: [@ Abort ]
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: