547142 - [OOPP] Exiting full-screen video locks the browser

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

(In reply to comment #15)
> While testing this, it goes into full screen ok while watching a video on Win7
> and stays.  But when I escape out of full screen now I'm get a complete browser
> hang after coming back out of Hulu fullscreen and no video, only audio
> continues playing.  I have to kill the browser.

Spin off from bug 539658. More focus hang fun.

Comment 1

[Jim Mathies [:jimm]]

Attachment: simplified hulu test case

going full screen on the second video and then exiting will usually trigger this.

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch

Comment 3

[Jim Mathies [:jimm]]

Attachment: patch

bsmedberg pointed out there's no need for the send message check. According to ReplyMessage docs, there's no effect if we're not in SendMessage.

Comment 4

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

Comment on attachment 427790 [details] [diff] [review]
patch

I like the changes you made to the switch statement but i don't think 0 is the correct thing to reply with in this case.

Comment 5

[Jim Jeffery not reading bug-mail 1/2/11]

This seems to not work.  Using the test-case and playing the 'windowless' video, and clicking into full-screen all is good until you ESC out.  The video/audio continues to play but the area where the small windowless vid should be, or was...is blank, then shortly thereafter the plugin will crash. 

Of course, it appears that the crashes for OOPP are not being sent since the new XUL crash page was implemented a few days ago. 

cset: http://hg.mozilla.org/mozilla-central/rev/23e78dba9f94 
Which should contain both patches.

Comment 6

[Jim Jeffery not reading bug-mail 1/2/11]

Seems that possible STR: 

Start video windowless test case or a video from HULU
While the video is playing mouse near the bottom to display playback controls
hit the pause button.
Press ESC, which returns to small window on Hulu page, but its blank..
Note that browser goes Not Responding, having to kill mozilla-runtime.

Win7 x64

Comment 7

[Jim Jeffery not reading bug-mail 1/2/11]

oops, sorry for the bug-spam - I was looking at wrong bug, this has not even been checked in yet :(

Comment 8

[[not reading bugmail]]

For comparison, I can get hulu to hang the browser on this page:
http://www.hulu.com/watch/60994/voltron-defender-of-the-universe-my-brother-is-a-robeast#s-p1-so-i0

If I watch the video on hulu, go to fullscreen, esc, go back to fullscreen, esc.  The browser completely hangs.

On the windowless testcase, go fullscreen, pause, esc, I get no video, a slight hang, the plugin crashes, the plugin ui comes up, and the browser comes back.

Comment 9

[Jim Mathies [:jimm]]

It's very easy to reproduce and diagnose. We ran into a problem with the first patch submitted but should have something landed here in a couple days that addresses this.

Comment 10

[Jim Mathies [:jimm]]

Attachment: patch

so getting windows nswindow header included was a bit of a dependency nightmare, so I used the send message trick we did with the focus stuff. Seems to work pretty well.

Comment 11

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

Comment on attachment 428287 [details] [diff] [review]
patch

>+  NS_ASSERTION(someWindow->sCallDepth >= 0, "Call depth out of sync.");

Since that's a static var let's use |nsWindow::sCallDepth| instead, otherwise it looks like a nonstatic member.

>+#ifdef MOZ_IPC
>+  someWindow->sCallDepth--;
>+#endif

Nit: indent that sCallDepth-- more.

>+        *(reinterpret_cast<UINT*>(wParam)) = sBaseMsg;

I think you should assert sCallDepth > 0 here, otherwise you may need to null out sBaseMsg when sCallDepth goes to 0.

Comment 12

[Jim Mathies [:jimm]]

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

Comment 13

[[not reading bugmail]]

V Fixed with latest hourly.

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

Comment 14

[[not reading bugmail]]

I tested the testcase multiple times and didn't see the bug, I tested the Hulu URL from before and didn't run into a problem.  Though I retested the URL again and I went in and out of full screen about 5-10 times trying to reproduce, which finally did.  

Unfortunately, I just tested a completely new video on hulu and saw the bug immediately - so still seeing the problem there.  I tried various timings from start to exit fullscreen, and reproduced the bug almost every time (20 times) except once. Testing Flash 10.0.45.2 - when I get a complete hang, and close Minefield, windows informs me that Adobe script is running slowly.  

Maybe hulu is more of a script based problem causing issue here and probably another bug?  I suppose a stack trace would be helpful.

Comment 15

[Benjamin Xiao]

I can confirm what Dale is saying. I am still getting hangs when exiting fullscreen Hulu videos. I have to manually terminate mozilla-runtime.exe to get the browser back. This bug should be reopened.

Comment 16

[Jim Mathies [:jimm]]

Debug builds don't appear to have any problems, but after a few tries a release build did freeze up for me. Looks like a completely different stack from the original posted unfortunately. Will post more info here in a bit.

Comment 17

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

Sounds like a new bug. Let's not reopen this one.

Comment 18

[Jim Mathies [:jimm]]

Hmm, I'm not having much luck, but I'm testing on my pokey laptop this week. I'll see if I can find another computer to test with. If anyone can reproduce this new hang easily, please post stacks of the parent & child if you can.

Comment 19

[Benjamin Xiao]

I can reproduce this everytime. How do I get stacks?

Comment 20

[Jim Mathies [:jimm]]

Attachment: stack v2.0

I was able to catch this, but I'm not sure what I've caught! The frames on the child side below flash were lost in windbg. The parent is handling WM_ACTIVATE, and appears to be sending a win event back to child, which ends up triggering a call back to parent over IPC. (just a guess at this point) Sort of a reverse of what we fixed the first time around.

Comment 21

[Jim Mathies [:jimm]]

(In reply to comment #19)
> I can reproduce this everytime. How do I get stacks?

If you're familiar with the windows debugger, you can use that. Download the windows debug tools, open windbg, and attach to firefox.exe. Do the same for mozilla-runtime.exe in another instance of windbg. You'll also need to setup the symbols server before hand - add

SRV*c:\symbols\*http://msdl.microsoft.com/download/symbols;SRV*c:\symbols\*http://symbols.mozilla.org/firefox

under file -> symbol file path... and save that out as a workspace so it's the default.

Comment 22

[Benjamin Xiao]

I downloaded the Windows Debugger, but everytime I try to attach firefox.exe, the browser becomes completely unresponsive so I can't even go to Hulu to create the crash.

Comment 23

[timeless]

https://developer.mozilla.org/En/How_to_get_a_stacktrace_with_windbg

Comment 24

[Benjamin Xiao]

When I follow the instructions posted by timeless, the browser doesn't even start up when I press Go. Instead I get this error.

Breakpoint 0 hit
eax=00000000 ebx=00000000 ecx=0017f8d0 edx=76f464f4 esi=fffffffe edi=00000000
eip=76f9e612 esp=0017f8ec ebp=0017f918 iopl=0         nv up ei pl zr na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00000246
ntdll!LdrpDoDebuggerBreak+0x30:
76f9e612 90              nop

Comment 25

[Jim Mathies [:jimm]]

Attachment: clean stack v2.0

Able to reproduce this locally now. Seems to help if you run a short video in full screen all the way to the end, then exit.

Comment 26

[Benjamin Xiao]

Attachment: firefox.exe stack

I finally managed to get a stack for the new fullscreen video bug. I hope this stack is useful.

Comment 27

[Benjamin Xiao]

Attachment: mozilla-runtime.exe stack

Here's the stack for mozilla-runtime.

Comment 28

[Jim Mathies [:jimm]]

I have this in a recording now and I've also managed to reproduce this in a debug build through some tricks I've learned. Currently I'm leaning toward hooking ShowWindow api calls in the child to address.  Tomorrow I'll either file off another bug or reopen this one once I'm done debugging the problem.

Comment 29

[Jim Mathies [:jimm]]

I've built some try builds of a new test patch in bug 550322 if anyone cares to take them for a spin. The new patch appears to address both hangs. If you can reproduce this easily, I'd appreciate help in testing this new patch to see if it addresses the problem.

Comment 30

[Benjamin Xiao]

I can confirm that your try build works fine on Hulu and on IGN videos. No crashes after repeated fullscreens and exits. Good work!

Comment 31

[[not reading bugmail]]

I agree, I also retested the simplified testcase here with the try server build in bug 550322 and this continues to work also for non hulu fullscreen hangs and video is there too when exiting back from fullscreen.

Everything looks like its working.