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)
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.
Updated•14 years ago
|
blocking2.0: --- → ?
Assignee | ||
Comment 1•14 years ago
|
||
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.
Comment 2•14 years ago
|
||
Blocking, and Jim, I'm assuming you were going to work on this one? If not, please reassign...
Assignee: nobody → jmathies
blocking2.0: ? → betaN+
Assignee | ||
Comment 3•14 years ago
|
||
(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
Assignee | ||
Comment 4•14 years ago
|
||
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
Assignee | ||
Comment 5•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #478868 -
Attachment is obsolete: true
Assignee | ||
Comment 6•14 years ago
|
||
Use Get/SetWindowLongPtrA explicitly in nsPluginNativeWindowWin. This insures pointer comparisons work right. Also did a little cleanup.
Assignee | ||
Comment 7•14 years ago
|
||
Fixes a focus event race with ipc disabled.
Assignee | ||
Comment 8•14 years ago
|
||
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.
Assignee | ||
Comment 9•14 years ago
|
||
Same thing for the ipc plugin code.
Assignee | ||
Comment 10•14 years ago
|
||
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)
Assignee | ||
Comment 11•14 years ago
|
||
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)
Assignee | ||
Comment 12•14 years ago
|
||
So WindowsDllInterceptor isn't 64-bit compat yet. So for now I'll disable the hook in 64-bit builds.
Assignee | ||
Comment 13•14 years ago
|
||
Attachment #479171 -
Attachment is obsolete: true
Assignee | ||
Comment 14•14 years ago
|
||
Attachment #479172 -
Attachment is obsolete: true
Comment 15•14 years ago
|
||
(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.
Assignee | ||
Comment 16•14 years ago
|
||
(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.
Assignee | ||
Comment 17•14 years ago
|
||
(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?
Assignee | ||
Comment 18•14 years ago
|
||
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.
Comment 19•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #479168 -
Attachment is obsolete: true
Attachment #479168 -
Flags: review?(jst)
Assignee | ||
Updated•14 years ago
|
Attachment #479229 -
Attachment is obsolete: true
Assignee | ||
Comment 20•14 years ago
|
||
Adding support for flash 10.1 wide subclasses.
Assignee | ||
Comment 21•14 years ago
|
||
Add support for wide api hooks.
Assignee | ||
Updated•14 years ago
|
Attachment #479314 -
Flags: review?(jst)
Assignee | ||
Comment 22•14 years ago
|
||
Comment on attachment 479230 [details] [diff] [review] oopp hook v.1 I'll add wide api support tomorrow.
Attachment #479230 -
Attachment is obsolete: true
Assignee | ||
Comment 23•14 years ago
|
||
Assignee | ||
Comment 24•14 years ago
|
||
a couple updates after some more testing.
Attachment #479317 -
Attachment is obsolete: true
Assignee | ||
Comment 25•14 years ago
|
||
Attachment #479469 -
Attachment is obsolete: true
Assignee | ||
Comment 26•14 years ago
|
||
jst, any feeling on this? Should I find other reviewers? These changes should land as soon as possible.
Updated•14 years ago
|
Attachment #479170 -
Flags: review?(jst) → review+
Assignee | ||
Updated•14 years ago
|
Attachment #479598 -
Flags: review?(jst)
Assignee | ||
Updated•14 years ago
|
Attachment #479599 -
Flags: review?(bent.mozilla)
Assignee | ||
Updated•14 years ago
|
Whiteboard: [verify bug 572417 once this lands]
Assignee | ||
Comment 27•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #479314 -
Flags: review?(jst) → review+
Updated•14 years ago
|
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+
Assignee | ||
Comment 30•14 years ago
|
||
updated.
Assignee | ||
Comment 31•14 years ago
|
||
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.
Assignee | ||
Comment 32•14 years ago
|
||
Attachment #483868 -
Attachment is obsolete: true
Assignee | ||
Comment 33•14 years ago
|
||
minus all the commented out, copy pasted code.
Attachment #484828 -
Attachment is obsolete: true
Assignee | ||
Comment 34•14 years ago
|
||
bah, with the right patch file this time!
Attachment #484831 -
Attachment is obsolete: true
Assignee | ||
Comment 35•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/e5f3177aa3bc http://hg.mozilla.org/mozilla-central/rev/0c356b93b352 http://hg.mozilla.org/mozilla-central/rev/3b04720c5c46 http://hg.mozilla.org/mozilla-central/rev/8db781d05f58
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 36•14 years ago
|
||
cc'ing Marcia and Chris. We should keep an eye on plugin related crashes on trunk post this landing.
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•