544518 - [OOPP] Crash killing plugin-container [@ txForwardContext::getVariable(int, nsIAtom*, txAExprResult*&) ] [@ mozilla::ipc::AsyncChannel::OnSend(IPC::Message*) ]

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[IU]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20100205 Minefield/3.7a1pre (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20100205 Minefield/3.7a1pre

Browser crashes when killing mozilla-runtime, after launching the linked URL.

http://crash-stats.mozilla.com/report/index/d1a8c23c-0778-4f65-929d-08fa52100205
http://crash-stats.mozilla.com/report/index/bp-3ac6d53c-6a83-4773-bc08-164962100205

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	txForwardContext::getVariable 	ipc/glue/AsyncChannel.cpp:429
1 	xul.dll 	RunnableMethod<mozilla::ipc::AsyncChannel,void 	ipc/chromium/src/base/task.h:307
2 	xul.dll 	MessageLoop::RunTask 	ipc/chromium/src/base/message_loop.cc:326
3 	xul.dll 	MessageLoop::DeferOrRunPendingTask 	ipc/chromium/src/base/message_loop.cc:334
4 	xul.dll 	MessageLoop::DoWork 	ipc/chromium/src/base/message_loop.cc:434
5 	xul.dll 	base::MessagePumpForIO::DoRunLoop 	ipc/chromium/src/base/message_pump_win.cc:446
6 	xul.dll 	base::MessagePumpWin::RunWithDispatcher 	ipc/chromium/src/base/message_pump_win.cc:52
7 	xul.dll 	base::MessagePumpWin::Run 	ipc/chromium/src/base/message_pump_win.h:78
8 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:194
9 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:168
10 	xul.dll 	base::Thread::ThreadMain 	ipc/chromium/src/base/thread.cc:165
11 	xul.dll 	`anonymous namespace'::ThreadFunc 	ipc/chromium/src/base/platform_thread_win.cc:26
12 	kernel32.dll 	BaseThreadStart


Reproducible: Always

Steps to Reproduce:
1. Configure Adobe Reader to NOT display in the browser.  Specifically, uncheck the "Display PDF in browser" option at: Edit --> Preferences... --> Internet
2. Close Adobe Reader
3. Create a new profile
4. Visit the linked URL
5. After the plugin launches and Adobe Reader displays the PDF, close Adobe Reader.
6. Kill associated mozilla-runtime.exe process
7. Browser crashes with reported signature.

Comment 1

[IU]

Something seems different, so the following slightly modified STR is needed to reproduce this bug:

1. Configure Adobe Reader to NOT display in the browser.  Specifically, uncheck
the "Display PDF in browser" option at: Edit --> Preferences... --> Internet
2. Close Adobe Reader
3. Create a new profile
4. Visit the linked URL
5. After the plugin launches and Adobe Reader displays the PDF, close Adobe
Reader.
6. If the browser had opened a new tab when performing Step 4, close that tab (this is a critical step)
7. Kill associated mozilla-runtime.exe process
8. Browser crashes with reported signature.

I will try and generate a stack trace log...

Comment 2

[IU]

I can't reproduce the crash when using WinDbg.  Nor can I get it to crash with the same signature.  Now I'm crashing with the PR_UNLOCK signature.  But also my STR is not quite right.  You pretty much have to run through Steps 4 to 7 twice, to get the crash.

Nevertheless, since I can't get the same signature and am now getting only the PR_UNLOCK signature, which already has a bug against it, I'm resolving this WORKSFORME

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a2pre) Gecko/20100217 Minefield/3.7a2pre (.NET CLR 3.5.30729) ID:20100217050024

Comment 3

[u88484]

I just got this crash using today's nightly.  I was reproducing bug 557609 and the browser crashed (usually it is just the plugin).  Upon restarting Firefox and checking about:crashes, I had two crash reports at the same time:

http://crash-stats.mozilla.com/report/index/c0b3e861-0fda-430c-b323-e32c22100407
http://crash-stats.mozilla.com/report/index/d41a3084-8623-4fbe-810b-4e7172100407

The first being this bug and the second being bug 557609.

Comment 4

[Benjamin Smedberg]

The crash is actually at AsyncChannel::OnSend(Message* aMsg)

Either "this" or "mTransport" is bogus. I'll bet mTransport, since the crash address is 0x0. cjones, can there be pending I/O events when we tear down a crashed plugin? And if so, who is supposed to cancel them?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Test (not to be checked in)

This is the first IPC bug I haven't been able to write an even reasonably probabilistic black-box IPDL/C++ test for.  Out of maybe 10 different timing strategies (including some tuned for my machine), I repro'd this bug once in maybe 100 runs (and one time in gdb by accident).  The problem is that to trigger the bug, OnError() needs to happen on the IO thread nearly simultaneously but just after a Send() on the worker thread, and NotifyError() needs to happen on the worker thread before OnSend() on the IO thread.  This patch adds a PR_Sleep() to AsyncChannel::OnError() to slow things down.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Don't think this is CRITICAL given the intricate timing needed to trigger this bug.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Send Messages directly through the Transport on the IO thread rather than through a no-added-value AsyncChannel indirection

Comment 8

[Benjamin Smedberg]

Technically this is "critical" because the definition of critical is a crasher bug, and the frequency of the crash isn't taken into account. Doesn't really matter, though. I don't see this much with 3.6.3plugin1 (only 4 reports), although it may be hiding under a different signature.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

bent, would be nice to get this reviewed soonish.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 438958 [details] [diff] [review]
Send Messages directly through the Transport on the IO thread rather than through a no-added-value AsyncChannel indirection

Looks good. Nice comment :)

Comment 11

[LegNeato]

Comment on attachment 438958 [details] [diff] [review]
Send Messages directly through the Transport on the IO thread rather than through a no-added-value AsyncChannel indirection

a=LegNeato for 1.9.2.4 (this is a blocker we want on the branch asap)

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Sorry, didn't ask for approval because I thought it needed to land on m-c first (and that's been exceedingly difficult of late).

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/e3cfb6ca54c9

Will let bake for a bit, land on branch ASAP, probably tomorrow.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ca6df0574ed6 (default)
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/c38bf492bef4 (relbranch)

Comment 15

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #1)
> Something seems different, so the following slightly modified STR is needed to
> reproduce this bug:
> 
> 1. Configure Adobe Reader to NOT display in the browser.  Specifically, uncheck
> the "Display PDF in browser" option at: Edit --> Preferences... --> Internet
> 2. Close Adobe Reader
> 3. Create a new profile
> 4. Visit the linked URL
> 5. After the plugin launches and Adobe Reader displays the PDF, close Adobe
> Reader.
> 6. If the browser had opened a new tab when performing Step 4, close that tab
> (this is a critical step)
> 7. Kill associated mozilla-runtime.exe process
> 8. Browser crashes with reported signature.
> 
> I will try and generate a stack trace log...

I can't get this to reproduce in build 1 of Firefox 3.6.4 (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.4) Gecko/20100413 Firefox/3.6.4 (.NET CLR 3.5.30729)).

Comment 16

[Al Billings [:abillings - ex-MoCo]]

IU, can you try verifying this fix with a nightly 1.9.2 build?

Comment 17

[IU]

I tried but was unable to confirm whether this fixed the issue.  I tried a build before this landed and couldn't get it to crash; however, I don't crash with this patch either.  Whatever fixed it, it's definitely fixed.