543764 - [OOPP] Crash [@ PR_Unlock] when ending mozilla-runtime at lastfm.com/listen

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Aakash Desai [:aakashd]]

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

Steps to Reproduce:
1. Go to www.lastfm.com/listen
2. After page load, end the "mozilla-runtime.exe" process

Actual Results:
The browser crashes.

Reports: 
http://crash-stats.mozilla.com/report/index/558967bc-25c4-4e78-ac3b-c7c702100202
http://crash-stats.mozilla.com/report/index/7098bcd2-f0b9-497a-9894-614112100202

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

It's not crashing for me.
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100202 Minefield/3.7a1pre

Comment 2

[Aakash Desai [:aakashd]]

Hm, so it might be an XP problem.

Comment 3

[Tony Chung [:tchung]]

repro steps in comment 0 sound similar to bug 543376, but the stack trace is different.   adding blocker dependency and flags.

Comment 4

[timeless]

Signature	PR_Unlock
UUID	558967bc-25c4-4e78-ac3b-c7c702100202
Crash Address	0x8

341 if (lock->owner != me) { 

PR_Unlock	 nsprpub/pr/src/threads/combined/prulock.c:341
mozilla::ipc::AsyncChannel::OnChannelError	ipc/glue/AsyncChannel.cpp:423
mozilla::ipc::RPCChannel::OnChannelError	ipc/glue/RPCChannel.cpp:463
IPC::Channel::ChannelImpl::OnIOCompleted	ipc/chromium/src/chrome/common/ipc_channel_win.cc:410
base::MessagePumpForIO::WaitForIOCompletion	
base::MessagePumpForIO::DoRunLoop	ipc/chromium/src/base/message_pump_win.cc:450
base::MessagePumpWin::RunWithDispatcher	ipc/chromium/src/base/message_pump_win.cc:52
base::MessagePumpWin::Run	ipc/chromium/src/base/message_pump_win.h:78
MessageLoop::RunHandler	ipc/chromium/src/base/message_loop.cc:194

Comment 5

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

I'm having trouble reproducing this because I keep hitting bug 543376 and bug 543839.  I'm afraid I'm going to have to come back to this bug after they're resolved.

Comment 6

[IU]

Attachment: WinDBG stack trace

This is based on the STR in Comment 0, with the exception that, before I load the lastfm.com site, I first load a page with flash so that mozilla-runtime.exe is launched.

Comment 7

[IU]

Comment on attachment 424947 [details]
WinDBG stack trace

Forget that.  I didn't do that one right.

Comment 8

[IU]

I reproduced this crash one, but now I too can no longer reproduce the crash.

Comment 9

[Johnny Stenback (:jst)]

Blocking whether this is still an existing crash or not.

Comment 10

[Benjamin Smedberg]

https://bugzilla.mozilla.org/show_bug.cgi?id=544365#c3 has possible STR and new crash reports:

http://crash-stats.mozilla.com/report/index/bp-0e3ab186-0b46-49be-8f50-869992100205
http://crash-stats.mozilla.com/report/index/bp-915154f6-5461-4db3-90c8-40a362100205

Comment 11

[IU]

I can now reproduce this crash with the following STR:

1. Go to: http://www.last.fm/videos
2. Wait for the page to completely load
3. Click the "Radio" link on the red bar at the top of that page
4. Kill mozilla-runtime.exe
5. Browser crashes with this signature

Attaching WinDBG log...

Comment 12

[IU]

Attachment: WinDBG log of crash

Comment 13

[Benjamin Smedberg]

I can't reproduce on Windows Vista given the last.fm STR. IU what version of flash are you using?

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

The only thing that happens for me when I kill mozilla-runtime on that page is that the plugin closes, and I have to refresh the page to get it to come back.
I'm running Windows 7 x64 with Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100205 Minefield/3.7a1pre 
Flash v10.0 r42

Comment 15

[IU]

(In reply to comment #13)
> I can't reproduce on Windows Vista given the last.fm STR. IU what version of
> flash are you using?

I'm running Windows XP and my plugin version is: 10.0.42.34

Comment 16

[IU]

http://crash-stats.mozilla.com/report/index/bp-db75465f-dbc6-4255-b489-65e2b2100205

Comment 17

[IU]

(In reply to comment #14)
> The only thing that happens for me when I kill mozilla-runtime on that page is
> that the plugin closes, and I have to refresh the page to get it to come back.

Based on your description, it does not appear you followed my STR correctly.  Your statement that, "the plugin closes, and I have to refresh the page to get it to come back" suggests you either performed Step 3 using a new tab or did something else different.

I'll update the STR and be very explicit...

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #17)
I just did it again a bit differently, still no crash.

First time I did it, after step 3 nothing on the page started playing, so I clicked the "Play" button, then proceeded to step 4.

This time, I just finished step 3, then when on to 4.

Comment 19

[IU]

(In reply to comment #18)
> (In reply to comment #17)
> I just did it again a bit differently, still no crash.
> 
> First time I did it, after step 3 nothing on the page started playing, so I
> clicked the "Play" button, then proceeded to step 4.
> 
> This time, I just finished step 3, then when on to 4.

You're still doing something different.  New STR coming up.

Comment 20

[IU]

Updated STR. Please follow exactly (DO NOT perform any clicks not listed or implied):

1. Create a new profile and launch Minefield with it.
2. Make sure dom.ipc.plugins.enabled = true
3. Make sure you have only one tab open.  Close all other tabs
4. Open Windows Task Manager and make sure there is absolutely no instance of mozilla-runtime.exe running.
5. Go to: http://www.last.fm/videos
6. Wait for the page to completely load, and do not click anything on the page
7. Left-click (i.e. "regular" click) the "Radio" link on the red bar at the top of that page, such that it loads in the very same tab.  You still should have no other tabs open, and none should have been launched
8. Using Windows Task Manager, got to the Processes tab and kill mozilla-runtime.exe
9. Browser crashes.  At least on XP it does.

Hope I haven't now missed some relevant detail. :-)

p.s. This is with the latest released build of Adobe Flash Player 10.0.42.34.

Comment 21

[IU]

Crap.  Step #5 should be: Go to http://www.lastfm.com/videos

Didn't catch it till I clicked "Commit" :-(

Comment 22

[IU]

Oops.  I see now that it was my error.  Sorry.  That explains why no one could reproduce the crash.  I committed the same error earlier.

Comment 23

[Benjamin Smedberg]

Topcrash according to crash-stats. bent, can you take (record, perhaps)?

Comment 24

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

Attachment: Patch

Got it. Small race where the IO thread isn't done posting the ChannelError message but the main thread goes ahead and processes the ChannelError anyway.

Comment 25

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

Comment on attachment 426759 [details] [diff] [review]
Patch

Score another point for record/replay.

Comment 26

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/4d8d4fd97c4f

Comment 27

[Benjamin Smedberg]

Backed out: this had hangs/deadlock-detector-assertions in test_hanging.html.

Windows-debug:
59 INFO TEST-PASS | /tests/modules/plugin/test/test_hanging.html | p.setColor should throw after the plugin crashes
###!!! ERROR: Potential deadlock detected:

Linux-opt was killed by the automation, had this stack:
 0  ld-2.5.so + 0x7f2
    eip = 0x00a6f7f2   esp = 0xbf9c3678   ebp = 0xbf9c36c8   ebx = 0xac11db80
    esi = 0x00000000   edi = 0xac11db80   eax = 0xfffffffc   ecx = 0x00000000
    edx = 0x00000002   efl = 0x00200202
    Found by: given as instruction pointer in context
 1  libnspr4.so!PR_Lock [ptsynch.c:4d8d4fd97c4f : 206 + 0x7]
    eip = 0x00793342   esp = 0xbf9c36d0   ebp = 0xbf9c36e8
    Found by: previous frame's frame pointer
 2  libxul.so!mozilla::ipc::AsyncChannel::NotifyMaybeChannelError() [Mutex.h : 103 + 0xa]
    eip = 0x016b4658   esp = 0xbf9c36f0   ebp = 0xbf9c3708
    Found by: previous frame's frame pointer
 3  libxul.so!mozilla::ipc::AsyncChannel::Close() [AsyncChannel.cpp:4d8d4fd97c4f : 170 + 0x8]
    eip = 0x016b4840   esp = 0xbf9c3710   ebp = 0xbf9c3738
    Found by: previous frame's frame pointer
 4  libxul.so!RunnableMethod<mozilla::plugins::PluginModuleParent, void (mozilla::plugins::PluginModuleParent::*)(), Tuple0>::Run() [tuple.h:4d8d4fd97c4f : 383 + 0xd]
    eip = 0x016af107   esp = 0xbf9c3740   ebp = 0xbf9c3758
    Found by: previous frame's frame pointer
 5  libxul.so!MessageLoop::RunTask(Task*) [message_loop.cc:4d8d4fd97c4f : 331 + 0x7]
    eip = 0x01760cd9   esp = 0xbf9c3760   ebp = 0xbf9c3788
    Found by: previous frame's frame pointer
 6  libxul.so!MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) [message_loop.cc:4d8d4fd97c4f : 339 + 0x9]
    eip = 0x01761171   esp = 0xbf9c3790   ebp = 0xbf9c37a8
    Found by: previous frame's frame pointer
 7  libxul.so!MessageLoop::DoWork() [message_loop.cc:4d8d4fd97c4f : 439 + 0xc]
    eip = 0x01761470   esp = 0xbf9c37b0   ebp = 0xbf9c37f8
    Found by: previous frame's frame pointer

Comment 28

[Benjamin Smedberg]

AsyncChannel::NotifyMaybeChannelError is being called from AsyncChannel::Close which is already holding the lock. It appears that the other caller, AsyncChannel::OnChannelError calls the method on a runnable which is not holding the lock. Hrm!

Comment 29

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

Attachment: Patch, v2

Don't lock if we're being called from close!

Comment 30

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

Attachment: Patch, v3

Split the methods rather than have a bool.

Comment 31

[Benjamin Smedberg]

Comment on attachment 427615 [details] [diff] [review]
Patch, v3

Please add a doccomment to the declaration of AsyncChannel::NotifyMaybeChannelError that it expects mMutex to be held, and may temporarily unlock it.

Comment 32

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

Attachment: Patch, v4

More better.

Comment 33

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

Comment on attachment 427634 [details] [diff] [review]
Patch, v4

Looks good, but there are several apparently unnecessary whitespace/return statement changes that I don't understand.

Comment 34

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

http://hg.mozilla.org/mozilla-central/rev/aec695f10003 fingers crossed!

Comment 35

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

(In reply to comment #33)
> Looks good, but there are several apparently unnecessary whitespace/return
> statement changes that I don't understand.

Er, crap. I didn't see that before I landed. Um, basically, |return FuncThatReturnsVoid();| just seems weird. Want me to revert?

Comment 36

[IU]

Fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a2pre) Gecko/20100219 Minefield/3.7a2pre ID:20100219043233
http://hg.mozilla.org/mozilla-central/rev/78cf81cafcff

However, it can still be crashed, but with slightly different steps and a different signature.  Filed Bug 547247 for that.

By the way, sorry for being an ass with comment 20. :-(

Comment 37

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

(In reply to comment #35)
> Want me to revert?

Not worth losing sleep or machine cycles over.