539658 - [OOPP] windowless flash doesn't work in full-screen mode (hulu)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

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

Opt build of e10s with the second v4 patch in bug 521377 applied.

STR
(1) Ensure OOPP is enabled
(2) Visit the URL above
(3) Click hulu's "Full Screen" button

The screen flashes, but then returns to non-full-screen mode.

Confirmed that hulu full-screen works for linux IPP/OPP, and IPP Windows 3.5.6.  Didn't check e10s-latest Windows IPP.

Comment 1

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

If it helps, after clicking "Full Screen" a few more times, I did see part of the hulu video appear scaled-up in the short-lived full-screen window before it disappeared.

Comment 2

[Jim Mathies [:jimm]]

Windowless only. My guess is flash creates a new window, parenting it to the browser in the other process. Then after going full screen, some api fails due to the cross process parent-child relationship, and flash falls out of full screen. This doesn't appear to have anything to do with the code in dom/plugins at this point.

Comment 3

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

On my Win7 x64 box I see the same issue, and this in Console2:

Error: store.free is not a function
Source file: http://static.huluim.com/system/hulu_48685.js
Line: 844

Comment 4

[Derek]

I have this same problem on justin.tv so I'm guessing this is the same bug? 

When clicking full screen on the justin.tv flash player it just flashes and stays windowed, Happens on windows 7 ultimate x64, so this don't just affect hulu.

Comment 5

[Jim Mathies [:jimm]]

(In reply to comment #2)
> Windowless only. My guess is flash creates a new window, parenting it to the
> browser in the other process. Then after going full screen, some api fails due
> to the cross process parent-child relationship, and flash falls out of full
> screen. This doesn't appear to have anything to do with the code in dom/plugins
> at this point.

Initially doesn't look like a win32 api failure, api monitor doesn't show anything interesting. Doing some more digging..

Comment 6

[Donald]

I have this problem on all flash video sites. Whenever I try to switch to full screen, it goes to full screen for less than a second before returning to normal. This is on the latest nightly build on Windows 7 x64.

Comment 7

[Benjamin Smedberg]

Yes, it's the same bug for any site that uses flash in windowless mode.

Comment 8

[Veritas]

why doesn't this affect youtube or other similar sites (www.southparkstudios.com/epidoes), e.g.?

i thought i was losing my mind -- glad this is an electrolysis problem, should be resolved in the near term.  is there any way to disable electrolysis right now?

Comment 9

[Brian Carpenter [:geeknik]]

(In reply to comment #8)
> is there any way to disable electrolysis right now?

Go into about:config and set dom.ipc.plugins.enabled to false.

Comment 10

[Jim Mathies [:jimm]]

Attachment: patch

Comment 11

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

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

>             break;
>+            case WM_KILLFOCUS:

Nit: newline between those

>+            {
>+              // When the user selects fullscreen mode in Flash video players,
>+              // WM_KILLFOCUS will be delayed by deferred process: WM_LBUTTONUP

Did you mean "by deferred message processing?"

>+              // WM_KILLFOCUS. Delayed delivery causes Flash to miss interpret

"misinterpret"

>+              // and drop it. (bug 539658)

I'm not a fan of adding bug numbers like this. We can trace back through blame to find it if needed.

>+              PRUnichar szClass[26];
>+              szClass[0] = '\0';

MSDN says GetClassName always null-terminates, so this should be unnecessary as long as you check GetClassName's return value.

>+              if (hwnd && GetClassNameW(hwnd, szClass, sizeof(szClass)/2) &&

I'd replace |2| with |sizeof(PRUnichar)| for safety.

Also, how about we add a | && hwnd != mPluginHWND| before calling GetClassName? That would maybe reduce the number of times we call GetClassName in practice.

>+              if (!CallNPP_HandleEvent(npremoteevent, &handled))
>+                  return 0;
>+            }
>+            break;

Seems like you could remove this duplication and just let it fallthrough to the default case...

Comment 12

[Jim Mathies [:jimm]]

Attachment: final

Comment 13

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

Comment on attachment 427600 [details] [diff] [review]
final

r=me

Comment 14

[Jim Mathies [:jimm]]

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

Comment 15

[[not reading bugmail]]

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.

Comment 16

[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.

Spun off bug 547142. I'll see if I can reproduce in the morning. Didn't see this on hulu during testing, but we also landed some focus work today, so I'll take a look.

Comment 17

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=11630 added to Litmus.