614979 - Flash Plugin Crashes [@ RtlEnterCriticalSection ]

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Scott A.]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101126 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101126 Firefox/4.0b8pre

For the past week or 2 I have been getting floods of crashes, this is the #1 top crasher right now on crash-stats.

Reproducible: Always

Steps to Reproduce:
1. Goto [@ RtlEnterCriticalSection ] 
2. Set play to 1080p, play, make it full screen.
3. Escape Fullscreen then reload page or close tab
4. Check about:crashes on tab close or see plugin crash on reload
Actual Results:  
Plugin crashes with RtlEnterCriticalSection

Expected Results:  
No Crash

http://crash-stats.mozilla.com/report/index/f5a3a1e6-23a0-4812-9f13-b53b92101126

File: NPSWF32.dll
Version: 10.1.102.64
Shockwave Flash 10.1 r102

Adapter Description: NVIDIA GeForce G102M
Vendor ID: 10de
Device ID: 0873
Adapter RAM: 512
Adapter Drivers: nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um
Driver Version: 8.17.12.6099
Driver Date: 10-16-2010
Direct2D Enabled: true
DirectWrite Enabled: true
GPU Accelerated Windows: 1/1 Direct3D 10

Comment 1

[Scott A.]

(In reply to comment #0)
> User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre)
> Gecko/20101126 Firefox/4.0b8pre
> Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre)
> Gecko/20101126 Firefox/4.0b8pre
> 
> For the past week or 2 I have been getting floods of crashes, this is the #1
> top crasher right now on crash-stats.
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 1. Goto http://www.youtube.com/watch?v=Cj6ho1-G6tw 
> 2. Set play to 1080p, play, make it full screen.
> 3. Escape Fullscreen then reload page or close tab
> 4. Check about:crashes on tab close or see plugin crash on reload
> Actual Results:  
> Plugin crashes with RtlEnterCriticalSection
> 
> Expected Results:  
> No Crash
> 
> http://crash-stats.mozilla.com/report/index/f5a3a1e6-23a0-4812-9f13-b53b92101126
> 
> File: NPSWF32.dll
> Version: 10.1.102.64
> Shockwave Flash 10.1 r102
> 
> Adapter Description: NVIDIA GeForce G102M
> Vendor ID: 10de
> Device ID: 0873
> Adapter RAM: 512
> Adapter Drivers: nvd3dumx,nvwgf2umx,nvwgf2umx nvd3dum,nvwgf2um,nvwgf2um
> Driver Version: 8.17.12.6099
> Driver Date: 10-16-2010
> Direct2D Enabled: true
> DirectWrite Enabled: true
> GPU Accelerated Windows: 1/1 Direct3D 10

Comment 2

[Alice0775 White]

Regression window(cached hourly):
Works;
http://hg.mozilla.org/mozilla-central/rev/c81efbbacc62
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101021 Firefox/4.0b8pre ID:20101021162014
Fails;
http://hg.mozilla.org/mozilla-central/rev/e5f3177aa3bc
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101021 Firefox/4.0b8pre ID:20101021160721
Pushlog;
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c81efbbacc62&tochange=e5f3177aa3bc

Comment 4

[Scott A.]

Steps to reproduce this one..

1. Load http://www.youtube.com/watch?v=Cj6ho1-G6tw
2. Change to 1080p, play, enter full screen
3. Exit full screen and reload the tab or close the tab
4. log crash

Comment 5

[Scott A.]

Commented to wrong bug but it applies on this duplicate too.

Comment 6

[Johnny Stenback (:jst)]

Jim, the regression window in comment 2 here indicates that this was triggered by your changes, but the bug this is marked a dupe of, which is itself marked a dupe of bug 556194 goes much farther back than the regression range that Alice found. Wondering if we've duped correctly here?

Comment 7

[Jim Mathies [:jimm]]

(In reply to comment #6)
> Jim, the regression window in comment 2 here indicates that this was triggered
> by your changes, but the bug this is marked a dupe of, which is itself marked a
> dupe of bug 556194 goes much farther back than the regression range that Alice
> found. Wondering if we've duped correctly here?

I need to chat with bsmedberg about that. Apparently there's a problem with freeing up these async queries, and my reorg of some of the setprop/getprop code made it worse. I think though if we fix bug 556194 the problem goes away.

Comment 8

[Benjamin Smedberg]

Jim triggered a preexisting bug. This is a proper dup.

Comment 9

[Benjamin Smedberg]

Or not, we're not clearing the window quickly enough after NPP_Destroy.

Comment 10

[Benjamin Smedberg]

Attachment: Destroy the window at NPP_Destroy, not instance destructor, rev. 1

Comment 11

[Jim Mathies [:jimm]]

(In reply to comment #10)
> Created attachment 496216 [details] [diff] [review]
> Destroy the window at NPP_Destroy, not instance destructor, rev. 1

This doesn't unhook the throttle subclass, or guarantee the flash message window is gone. I'd suggest turning off our throttling logic as well to be safe.

Did this fix the crash?

Comment 12

[Benjamin Smedberg]

Yes, this fixes the crash. I'm not sure what you mean about the throttle subclass, because it destroys the window so it doesn't really matter, right?

Comment 13

[Jim Mathies [:jimm]]

Comment on attachment 496216 [details] [diff] [review]
Destroy the window at NPP_Destroy, not instance destructor, rev. 1

(In reply to comment #12)
> Yes, this fixes the crash. I'm not sure what you mean about the throttle
> subclass, because it destroys the window so it doesn't really matter, right?

I just want to be sure flash destroys the message window when we destroy the plugin window. If that's the case, great, and that would explain why this fixes the problem.

Comment 14

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/22bb9d6626c5 (landed on Thursday)

Comment 15

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if  you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 16

[Scott A.]

(In reply to comment #15)
> As per today's meeting, beta 9 will be a time-based release. Marking these all
> betaN+. Please move it back to beta9+ if  you believe it MUST be in the next
> beta (ie: trunk is in an unshippable state without this)

Why did it even have a Beta9 status? This bug is marked as fixed since before Beta 8.