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

RESOLVED FIXED in mozilla2.0b12

Status

()

Core
Plug-ins
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: jimm, Assigned: Benjamin Smedberg)

Tracking

Trunk
mozilla2.0b12
x86
Windows Vista
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [hardblocker])

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
#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.
(Reporter)

Updated

8 years ago
Assignee: nobody → jmathies
(Assignee)

Comment 1

8 years ago
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]
(Reporter)

Comment 2

8 years ago
(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.
(Reporter)

Comment 3

8 years ago
Created attachment 509257 [details] [diff] [review]
subclass safeguard patch v.1
(Reporter)

Comment 4

8 years ago
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)
(Assignee)

Updated

8 years ago
Attachment #509257 - Flags: review?(benjamin) → review+
(Reporter)

Updated

8 years ago
Keywords: checkin-needed

Comment 5

8 years ago
http://hg.mozilla.org/mozilla-central/rev/3aecade382af
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b12

Comment 6

8 years ago
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
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
(Reporter)

Updated

8 years ago
Summary: Flash crash [@ _SEH_prolog ] → Flash crash [@ _SEH_prolog ] | [@ _SEH_prolog4 ]
(Reporter)

Comment 9

8 years ago
Not fixed yet!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Summary: Flash crash [@ _SEH_prolog ] | [@ _SEH_prolog4 ] → Flash crash [@ _SEH_prolog ] | [@ _SEH_prolog4 ] | [@ InternalCallWinProc ]
(Reporter)

Updated

8 years ago
Duplicate of this bug: 631995
(Reporter)

Comment 11

8 years ago
The different signatures match to different os versions, but they are all the same crash.

Updated

8 years ago
Depends on: 632651

Updated

8 years ago
Duplicate of this bug: 632790

Updated

8 years ago
Duplicate of this bug: 632819
(Reporter)

Comment 14

8 years ago
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
(Assignee)

Comment 15

8 years ago
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
(Assignee)

Updated

8 years ago
Blocks: 632790
(Reporter)

Comment 16

8 years ago
(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. ;)
(Reporter)

Updated

8 years ago
Blocks: 627012
(Assignee)

Comment 17

8 years ago
Created attachment 511194 [details] [diff] [review]
Set WindowProc using the unicode function, rev. 1
Assignee: jmathies → benjamin
Status: REOPENED → ASSIGNED
Attachment #511194 - Flags: review?(jmathies)
(Assignee)

Comment 18

8 years ago
This is a harblocker via becoming a topcrash in Flash 10.2
Whiteboard: [softblocker] → [hardblocker]
(Assignee)

Updated

8 years ago
Duplicate of this bug: 632790
(Reporter)

Updated

8 years ago
Attachment #511194 - Flags: review?(jmathies) → review+
(Reporter)

Comment 20

8 years ago
http://hg.mozilla.org/mozilla-central/rev/835b80d17417

That should settle things down quite a bit.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED

Updated

8 years ago
No longer blocks: 632790

Updated

8 years ago
Blocks: 632819
(Assignee)

Updated

8 years ago
Duplicate of this bug: 632651

Updated

8 years ago
Duplicate of this bug: 632819

Updated

8 years ago
No longer blocks: 632819
(Assignee)

Updated

7 years ago
Duplicate of this bug: 632943
You need to log in before you can comment on or make changes to this bug.