Closed Bug 596094 Opened 14 years ago Closed 14 years ago

windowed plugin subclass is dropped after entering/exiting full screen in flash

Categories

(Core Graveyard :: Plug-ins, defect)

x86
Windows Vista
defect
Not set
normal

Tracking

(blocking2.0 betaN+)

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: [verify bug 572417 once this lands])

Attachments

(6 files, 11 obsolete files)

2.09 KB, patch
jst
: review+
Details | Diff | Splinter Review
8.15 KB, patch
jst
: review+
Details | Diff | Splinter Review
8.19 KB, patch
jst
: review+
vlad
: review+
Details | Diff | Splinter Review
17.05 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
21.65 KB, patch
Details | Diff | Splinter Review
9.92 KB, patch
Details | Diff | Splinter Review
str:

1) open youtube video
2) go to full screen
3) exit full screen

check PluginInstanceChild's PluginWindowProc to see if it's receiving any event traffic. I'm not seeing it.

Also, on DestroyPluginWindow(), wndProc will point to DefWndProc.
Blocks: OOPP
blocking2.0: --- → ?
Interesting, this is also a problem with ipc disabled. Resizing the view to clip the plugin will trigger a set window call, resetting the subclass.

I wonder if this is due to new code in flash. I don't remember ever seeing this before.
Blocking, and Jim, I'm assuming you were going to work on this one? If not, please reassign...
Assignee: nobody → jmathies
blocking2.0: ? → betaN+
(In reply to comment #2)
> Blocking, and Jim, I'm assuming you were going to work on this one? If not,
> please reassign...

yep, working on this now. I need to get this fixed before I can tackle bug 572417.
Blocks: 572417
This is not just OOPP, it happens with ipc disabled. It's really quite strange, resetting the subclass after we CallWindowProc doesn't help. There's something wrong with the subclass chain. Also the event that triggers this is always WM_IME_SETCONTEXT. Another oddity - this event is sent in a number of cases, but the subclass loss only happens when flash goes into fullscreen mode.
Summary: [OOPP] PluginInstanceChild windowed subclass is dropped after entering/exiting full screen in flash → windowed plugin subclass is dropped after entering/exiting full screen in flash
No longer blocks: OOPP
Attached patch fix for event race v.1 (obsolete) — Splinter Review
Something I found a long the way, we sometimes experience an event race when we forward events down to widget. This is with ipc disabled.
Attachment #478868 - Attachment is obsolete: true
Attached patch pluginwin prep v.1 (obsolete) — Splinter Review
Use Get/SetWindowLongPtrA explicitly in nsPluginNativeWindowWin. This insures pointer comparisons work right. Also did a little cleanup.
Attached patch race fix v.1Splinter Review
Fixes a focus event race with ipc disabled.
Attached patch pluginwin hook (obsolete) — Splinter Review
Hook SetWindowLongA/SetWindowLongPtrA so we can catch flash resetting our subclass. Note they are doing this from the event procedure of the fullscreen window, so there's no way to trap for this in PluginWndProc.
Attached patch oopp hook v.1 (obsolete) — Splinter Review
Same thing for the ipc plugin code.
Comment on attachment 479168 [details] [diff] [review]
pluginwin prep v.1

Kicking off initial review of the prep changes. Aside from the misc. cleanup (which I can separate out if need be) the point here is to explicitly use the ascii api version of set and get. When you mix and match the ascii and wide you get back pointers that can be tokens that represent the procedure you set. This was interfering with procedure comparisons in SubclassAndAssociateWindow.

Johnny, you're the official peer here if you want to review this, otherwise feel free to reassign. Bent can do the oop code review (and maybe the first hook patch as well). Might be good to get vlad to do an sr on the use of the hook code.

There are other ways I can think of to accomplish this (subclassing the flash fullscreen window for example) but this seemed the most straight forward solution. The hook stays active for the lifetime of the app, but since we only use the ascii version of SetWindowLongPtrA for plugin widgets, the hook only gets called for the specific case we're interested in.
Attachment #479168 - Flags: review?(jst)
Comment on attachment 479170 [details] [diff] [review]
race fix v.1

Unrelated but I came across this while working on things. With ipc disabled we can get into a situation where the focus manager triggers a set focus event, which ends up in here. We then call down to widget which goes to the focus manager again, triggering the same event.
Attachment #479170 - Flags: review?(jst)
So WindowsDllInterceptor isn't 64-bit compat yet. So for now I'll disable the hook in 64-bit builds.
Attached patch pluginwin hook v.1 (obsolete) — Splinter Review
Attachment #479171 - Attachment is obsolete: true
Attached patch oopp hook v.1 (obsolete) — Splinter Review
Attachment #479172 - Attachment is obsolete: true
(In reply to comment #6)
> Use Get/SetWindowLongPtrA explicitly in nsPluginNativeWindowWin. This insures
> pointer comparisons work right. Also did a little cleanup.
Why Is this required? This will regress bug 496630.
(In reply to comment #15)
> (In reply to comment #6)
> > Use Get/SetWindowLongPtrA explicitly in nsPluginNativeWindowWin. This insures
> > pointer comparisons work right. Also did a little cleanup.
> Why Is this required? This will regress bug 496630.

Well it's nice to know folks from Adobe are tuned in, thanks for commenting. Our initial subclass in widget is ascii, we do that with all plugins. (I believe the reason we do this is due to older flash versions which require it, see bug 179822) Subsequent subclasses are wide via the nativewindow code, which switched to wide when we introduced ipc. But I felt that was in conflict with our original ascii subclass. So I've updated the code to use ascii in nativewindow subclassing.

If certain versions of flash now require wide character subclasses, it would be great if you could give us that information so we could compensate for it in the code. 

Outside of this, maybe you could explain why Flash now seems to reset our subclass when entering and exiting fullscreen? I believe that's something Adobe recently added, which breaks code that compensates for lost focus events, and prevents flash's wm_user+x events from drowning our native event processing.
(In reply to comment #15)
> (In reply to comment #6)
> > Use Get/SetWindowLongPtrA explicitly in nsPluginNativeWindowWin. This insures
> > pointer comparisons work right. Also did a little cleanup.
> Why Is this required? This will regress bug 496630.

Doh, sorry Masatoshi, I thought you worked for Adobe! Maybe we should cc Jeff Mott and get his input?
This is good news actually, we can switch to wide character apis on 10.1 and later. I'll update tho hook patches to address this.
I'm not an Adobe employee, but according to bug 496630 comment 12, Flash Player 10.1 or later subclass the window using wide functions.
If some others (including us) subclass it over using narrow functions, Unicode characters will be broken by Windows system. So we need use wide functions when we subclass after Flash Player. We don't have to change the initial subclass because nobody care about what is passed to the initial wndproc. Using narrow functions for initial subclass will keep the maximum compatibility with old plug-ins which expect initial subclass is narrow.
FYI, we don't have to check whether Flash Player uses narrow functions because CallWindowProcW and Get/SetWindowLongPtr will do the magic on behalf of us.
Attachment #479168 - Attachment is obsolete: true
Attachment #479168 - Flags: review?(jst)
Attachment #479229 - Attachment is obsolete: true
Adding support for flash 10.1 wide subclasses.
Attached patch pluginwin hook v.2 (obsolete) — Splinter Review
Add support for wide api hooks.
Attachment #479314 - Flags: review?(jst)
Comment on attachment 479230 [details] [diff] [review]
oopp hook v.1

I'll add wide api support tomorrow.
Attachment #479230 - Attachment is obsolete: true
Attached patch oopp hook v.2 (obsolete) — Splinter Review
a couple updates after some more testing.
Attachment #479317 - Attachment is obsolete: true
Attached patch oopp hook v.3Splinter Review
Attachment #479469 - Attachment is obsolete: true
jst, any feeling on this? Should I find other reviewers? These changes should land as soon as possible.
Attachment #479170 - Flags: review?(jst) → review+
Attachment #479598 - Flags: review?(jst)
Attachment #479599 - Flags: review?(bent.mozilla)
Whiteboard: [verify bug 572417 once this lands]
Comment on attachment 479598 [details] [diff] [review]
pluginwin hook v.3

Vlad, requesting you look over these hooks, since your worked on the original dll interceptor.
Attachment #479598 - Flags: review?(vladimir)
Attachment #479314 - Flags: review?(jst) → review+
Attachment #479598 - Flags: review?(jst) → review+
Comment on attachment 479598 [details] [diff] [review]
pluginwin hook v.3

looks reasonable
Attachment #479598 - Flags: review?(vladimir) → review+
Comment on attachment 479599 [details] [diff] [review]
oopp hook v.3

>+PluginInstanceChild::HookGuts(HWND hWnd,
> ...
>+  if (nIndex != GWLP_WNDPROC)
>+    return PR_TRUE;
>+  // if it's not a subclassed plugin window
>+  else if (!GetProp(hWnd, kPluginInstanceChildProperty))

Please yank all these else-after-returns.

>+  printf("*** preventing!\n");

And this ;)

Otherwise looks ok. We should really ask Adobe to contact us and ask for more NPAPI hooks rather than trying to play more tricks with our wndproc...
Attachment #479599 - Flags: review?(bent.mozilla) → review+
Attached patch oopp hook v.4 (obsolete) — Splinter Review
updated.
Attached patch pluginwin hook v.4 (obsolete) — Splinter Review
I did some testing on older version of flash (back to the original version 9) and found an issue with returning default procedures when we block a set. So I've updated both patches to - 

1) set the flash subclass and save the outgoing procedure
2) set our subclass and store the outgoing procedure in our old wnd proc
3) return the outgoing procedure flash is expecting

I also added some code to disable this when entering Destroy in the plugin-container which was causing random flash crashes on shutdown.
Attached patch oopp hook v.5Splinter Review
Attachment #483868 - Attachment is obsolete: true
Attached patch pluginwin hook v.5 (obsolete) — Splinter Review
minus all the commented out, copy pasted code.
Attachment #484828 - Attachment is obsolete: true
bah, with the right patch file this time!
Attachment #484831 - Attachment is obsolete: true
cc'ing Marcia and Chris. We should keep an eye on plugin related crashes on trunk post this landing.
Depends on: 606846
Depends on: 614979
Depends on: 607299
Depends on: 556194
Depends on: 772842
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.