Closed Bug 630697 Opened 9 years ago Closed 9 years ago

Flash crash [@ _SEH_prolog ] | [@ _SEH_prolog4 ] | [@ InternalCallWinProc ]

Categories

(Core :: Plug-ins, defect)

x86
Windows Vista
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b12
Tracking Status
blocking2.0 --- final+

People

(Reporter: jimm, Assigned: benjamin)

References

Details

(Whiteboard: [hardblocker])

Attachments

(2 files)

#10 top crasher in beta 10.

This is an avoidable bug in our subclassing - 

0 	user32.dll 	_SEH_prolog 	
1 	user32.dll 	CallWindowProcAorW 	
2 	user32.dll 	CallWindowProcW 	
3 	xul.dll 	mozilla::plugins::PluginInstanceChild::PluginWindowProc 	dom/plugins/PluginInstanceChild.cpp:1156
4 	user32.dll 	InternalCallWinProc 	
5 	user32.dll 	UserCallWinProcCheckWow 	
6 	user32.dll 	CallWindowProcAorW 	
7 	user32.dll 	CallWindowProcW 	
8 	xul.dll 	mozilla::plugins::PluginInstanceChild::PluginWindowProc 	dom/plugins/PluginInstanceChild.cpp:1156


LRESULT res = CallWindowProc(self->mPluginWndProc, hWnd, message, wParam,
 lParam); 

self->mPluginWndProc somehow ends up pointing to our own procedure.
Assignee: nobody → jmathies
No STR, I'm guessing? I assume this only happens as we're tearing down a plugin instance? I'd ship with this if we had to because it's a plugin-process crash, but yeah, we should fix it.
blocking2.0: ? → final+
Whiteboard: [softblocker]
(In reply to comment #1)
> No STR, I'm guessing? I assume this only happens as we're tearing down a plugin
> instance? I'd ship with this if we had to because it's a plugin-process crash,
> but yeah, we should fix it.

No str, but I think I see where this could happen. If they set the subclass to our procedure, then change it to another procedure and keep running, we could end up in this state. We expect them to do this only once on tear down, but it seems that has changed. I have a patch I'm testing that might address this.
Comment on attachment 509257 [details] [diff] [review]
subclass safeguard patch v.1

bsmedberg, this is a pretty safe patch to take. In our subclass hook, I'm just adding checks to be sure we don't assign the subclass we hold to our own procedure. I don't have an str but this code seems to be the only place this could possibly get mixed up.
Attachment #509257 - Flags: review?(benjamin)
Attachment #509257 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/3aecade382af
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12
I backed this patch as part of this pushlog <http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7e12e3e16e6c> because of the oranges it caused <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296852850.1296854923.2345.gz&fulltext=1>.

I'm not sure which one of the bugs was at fault, so I just backed them all out.  The assignee needs to investigate and make sure that his patch has not been the culprit, and then re-add checkin-needed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Relanded all but one of the patches in that push.  For this bug, that's:
http://hg.mozilla.org/mozilla-central/rev/c1d4dc902b7a
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Summary: Flash crash [@ _SEH_prolog ] → Flash crash [@ _SEH_prolog ] | [@ _SEH_prolog4 ]
Not fixed yet!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Flash crash [@ _SEH_prolog ] | [@ _SEH_prolog4 ] → Flash crash [@ _SEH_prolog ] | [@ _SEH_prolog4 ] | [@ InternalCallWinProc ]
Duplicate of this bug: 631995
The different signatures match to different os versions, but they are all the same crash.
Depends on: 632651
Duplicate of this bug: 632790
Duplicate of this bug: 632819
Hmm, I'm at a loss here, we don't assign mPluginWndProc in many places, and in each case where we do we check to make sure it's not going to get PluginWndProc.

http://mxr.mozilla.org/mozilla-central/search?find=%2Fdom%2Fplugins%2F&string=mPluginWndProc
I loaded a minidump: the message in this particular minidump is WM_CAPTURECHANGED. Unfortunately even MSVC won't walk all the way up the stack to show me what's at the top.

What about http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginInstanceChild.cpp#955 ? Are we sure that GetWindowLongPtr and SetWindowLongPtr do the right thing?

Can we implement a "dumb" assertion and runtime check here? http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginInstanceChild.cpp#1208
Blocks: 632790
(In reply to comment #15)
> I loaded a minidump: the message in this particular minidump is
> WM_CAPTURECHANGED. Unfortunately even MSVC won't walk all the way up the stack
> to show me what's at the top.
> 
> What about
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginInstanceChild.cpp#955
> ? Are we sure that GetWindowLongPtr and SetWindowLongPtr do the right thing?

Well, the get should return the current procedure, and our set hook specifically checks for PluginWindowProc and lets it through.

http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginInstanceChild.cpp#1281

We don't have problems with this in 3.6 so this code should be working right.

> 
> Can we implement a "dumb" assertion and runtime check here?
> http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginInstanceChild.cpp#1208

Certainly. We can totally kludge a fix by dumping messages to defwndproc here if we want to. There may be side effects to this, but at least it won't crash. It's wallpaper though. ;)
Blocks: 627012
Assignee: jmathies → benjamin
Status: REOPENED → ASSIGNED
Attachment #511194 - Flags: review?(jmathies)
This is a harblocker via becoming a topcrash in Flash 10.2
Whiteboard: [softblocker] → [hardblocker]
Duplicate of this bug: 632790
Attachment #511194 - Flags: review?(jmathies) → review+
http://hg.mozilla.org/mozilla-central/rev/835b80d17417

That should settle things down quite a bit.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
No longer blocks: 632790
Blocks: 632819
Duplicate of this bug: 632651
Duplicate of this bug: 632819
No longer blocks: 632819
Duplicate of this bug: 632943
You need to log in before you can comment on or make changes to this bug.