805745 - crash in nsWindow::OnPaint

Status: VERIFIED FIXED

(Core :: Widget: Win32, defect)

Description

[Scoobidiver (away)]

It first appeared in 19.0a1/20121025. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=93cc1ee94291&tochange=5c82f5a5e90d

Signature 	mozilla::layers::ImageContainer::Release() More Reports Search
UUID	08f4bce0-c20d-4cee-bee6-221822121026
Date Processed	2012-10-26 07:17:09
Uptime	16
Install Age	16 seconds since version was first installed.
Install Time	2012-10-26 07:16:28
Product	Firefox
Version	19.0a1
Build ID	20121025030620
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	AuthenticAMD family 16 model 6 stepping 2
Crash Reason	EXCEPTION_ACCESS_VIOLATION_WRITE
Crash Address	0x740075
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x68e0, AdapterSubsysID: 00000000, AdapterDriverVersion: 8.713.3.0
D3D10 Layers? D3D10 Layers- 
Processor Notes 	This dump is too long and has triggered the automatic truncation routine
EMCheckCompatibility	True
Adapter Vendor ID	0x1002
Adapter Device ID	0x68e0
Total Virtual Memory	4294836224
Available Virtual Memory	3903037440
System Memory Use Percentage	55
Available Page File	2922856448
Available Physical Memory	951963648

Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::layers::ImageContainer::Release 	obj-firefox/dist/include/ImageContainer.h:265
1 	xul.dll 	nsRefPtr<mozilla::layers::ImageContainer>::~nsRefPtr<mozilla::layers::ImageConta 	obj-firefox/dist/include/nsAutoPtr.h:874
2 	xul.dll 	mozilla::layers::ImageLayer::~ImageLayer 	gfx/layers/ImageLayers.cpp:18
3 	xul.dll 	mozilla::layers::ImageLayer::`scalar deleting destructor' 	
4 	xul.dll 	nsWindow::OnPaint 	widget/windows/nsWindowGfx.cpp:420
5 	xul.dll 	nsWindow::ProcessMessage 	widget/windows/nsWindow.cpp:4727
6 	xul.dll 	nsWindow::WindowProcInternal 	widget/windows/nsWindow.cpp:4336
7 	xul.dll 	CallWindowProcCrashProtected 	xpcom/base/nsCrashOnException.cpp:32
8 	xul.dll 	nsWindow::WindowProc 	widget/windows/nsWindow.cpp:4282
9 	user32.dll 	InternalCallWinProc 	
10 	user32.dll 	NtUserGetDC 	
11 	user32.dll 	DispatchClientMessage 	
12 	user32.dll 	__fnDWORD 	
13 	ntdll.dll 	KiUserCallbackDispatcher 	
14 	ntdll.dll 	KiUserApcDispatcher 	
15 	xul.dll 	nsWindow::Show 	widget/windows/nsWindow.cpp:1189
16 	xul.dll 	nsXULWindow::SetVisibility 	xpfe/appshell/src/nsXULWindow.cpp:795
17 	xul.dll 	nsXULWindow::OnChromeLoaded 	xpfe/appshell/src/nsXULWindow.cpp:992
18 	xul.dll 	nsWebShellWindow::OnStateChange 	xpfe/appshell/src/nsWebShellWindow.cpp:563
19 	xul.dll 	nsDocLoader::DoFireOnStateChange 	uriloader/base/nsDocLoader.cpp:1351
20 	xul.dll 	nsDocLoader::doStopDocumentLoad 	uriloader/base/nsDocLoader.cpp:942
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Alayers%3A%3AImageContainer%3A%3ARelease%28%29

Comment 1

[Scoobidiver (away)]

It might be a regression from bug 782547.

Comment 2

[Scoobidiver (away)]

With combined signatures, it's #3 top crasher over the last three days.

Comment 3

[Neil Deakin]

(In reply to Scoobidiver from comment #1)
> It might be a regression from bug 782547.

Did you mean that that bug caused this crash to occur more frequently? That patch was only in for a few days and the crash reports show crashes over a wide range of different builds.

Comment 4

[Scoobidiver (away)]

(In reply to Neil Deakin from comment #3)
> Did you mean that that bug caused this crash to occur more frequently?
I suspect it but it might be another one as no lines in the stack trace have been changed by one of the patch in the regression range. 

> the crash reports show crashes over a wide range of different builds.
But startup crashes have happened since October 25th.
More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsBaseWidget%3A%3AAutoLayerManagerSetup%3A%3A~AutoLayerManagerSetup%28%29
https://crash-stats.mozilla.com/report/list?signature=_moz_cairo_destroy
https://crash-stats.mozilla.com/report/list?signature=nsWindow%3A%3AOnPaint%28HDC__*%2C+unsigned+int%29
https://crash-stats.mozilla.com/report/list?signature=gfxASurface%3A%3ARelease%28%29
https://crash-stats.mozilla.com/report/list?signature=gfxContext%3A%3A~gfxContext%28%29
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Alayers%3A%3ALayer%3A%3A~Layer%28%29

Comment 5

[Robert Kaiser]

gfxASurface::Release() saw a significant increase on 2012-10-31, we really need to get some traction on those issues.

Comment 6

[Daniel Veditz [:dveditz]]

gfxASurface::Release() saw a first peak starting Oct 29, then decreased before ramping up again. Two separate bugs? A backout/re-landing? People taking time for Halloween?

gfxASurface::Release() seems to be a consistent READ access violation at 0x15 so looks like a null deref, but it's trying to read member mSurfaceValid which means "this" is null-- how does that happen? Maybe the surface is a member of some other structure and this is a use-after-free of that object? There could well be a security problem lurking here.

Comment 7

[Alex Keybl [:akeybl]]

Neil - can we try temporarily backing out bug 782547 given it's our best guess as to the regressing bug right now?

Also adding roc/vlad as the module owners for Core::Widget, and adding qawanted/needURLs to see if we're able to repro internally.

Comment 8

[Neil Deakin]

The patch in bug 782547 was only in from around Oct 24 and was backed out on Oct 27.

Comment 10

[Leman Bennett [Omega]]

I ran into this crash using Ubisoft's Shop order form. It practically stops you from buying anything from the store using Firefox due to the crash. It happens as soon as you click on a drop down to input data. Confirmed on both Nightly and Fx17.

Comment 11

[Scoobidiver (away)]

(In reply to Leman Bennett (Omega X) from comment #10)
> Confirmed on both Nightly and Fx17.
So it's not this bug because 17.0 and 18.0 are unaffected.
What are your crash IDs?

Comment 12

[Leman Bennett [Omega]]

https://crash-stats.mozilla.com/report/index/bp-cbd938cf-9aa5-445c-8a39-d80e82121127
https://crash-stats.mozilla.com/report/index/bp-30420c7e-4fe8-4394-9e9f-c518f2121127
https://crash-stats.mozilla.com/report/index/bp-c121ca79-daa6-4217-bf48-13b592121127

Despite the difference in signature, all crashes were the exact same way. Select the dropdown, then crash.

Comment 13

[Scoobidiver (away)]

(In reply to Leman Bennett (Omega X) from comment #12)
> https://crash-stats.mozilla.com/report/index/bp-cbd938cf-9aa5-445c-8a39-
> d80e82121127
> https://crash-stats.mozilla.com/report/index/bp-30420c7e-4fe8-4394-9e9f-
> c518f2121127
> https://crash-stats.mozilla.com/report/index/bp-c121ca79-daa6-4217-bf48-
> 13b592121127
It's bug 716844 and bug 793983 (likely a dupe of bug 716844), not this bug.

Comment 14

[Leman Bennett [Omega]]

I doubt that this is a 10 month old crash. Maybe 793983.

Comment 15

[u279076]

Marcia, have you been able to reproduce this crash?

Comment 16

[Bas Schouten (:bas.schouten)]

I can reproduce this on Fx17 but -not- on a debug build of nightly. I'll look into this a little more.

Comment 17

[Bas Schouten (:bas.schouten)]

I strongly suspect this might be related to bug 743975

Comment 18

[u279076]

Bas, can you provide more details about how you reproduce this issue?

Comment 19

[Bas Schouten (:bas.schouten)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #18)
> Bas, can you provide more details about how you reproduce this issue?

1. Go to http://shop.ubi.com/
2. Select platform 'PC'
3. Pre-order FarCry 3
4. Go to check-out
5. Select one of the drop-down listboxes

This reproduces in Optimized builds -not- in unoptimized builds.

Comment 20

[Bas Schouten (:bas.schouten)]

I'm marking this security sensitive. Putting more details in a restricted comment.

Comment 27

[Bas Schouten (:bas.schouten)]

The problem has been rootcaused and I've talked to several people about possible ways of fixing it but it's hard.

Re-assigning to Timothy Nikkel as he knows a lot more about this code and is on the review list for one of the possible causes of this issue.

Comment 28

[Timothy Nikkel (:tnikkel)]

The crash of comment 19 is almost certainly caused by bug 782980. It added the ProcessPendingUpdates call in nsViewManager::Refresh which flushes and can therefore destroy anything.

Comment 29

[Robert Kaiser]

gfxASurface::Release() is the #3 topcrash in 20.0a1 and 19.0a2 - see https://crash-stats.mozilla.com/report/list?signature=gfxASurface%3A%3ARelease%28%29&version=Firefox%3A19.0a2 - can we please get some progress here? Timothy? Bas?

Comment 30

[u279076]

(In reply to Bas Schouten (:bas.schouten) from comment #19)
> This reproduces in Optimized builds -not- in unoptimized builds.

Is there somewhere I can get optimized builds to try to confirm a regression range? or is qawanted no longer needed on this to get a fix?

Comment 31

[Timothy Nikkel (:tnikkel)]

No, I don't think we need any qa help with this right now.

Comment 32

[Timothy Nikkel (:tnikkel)]

(In reply to Robert Kaiser (:[e-mail]) from comment #29)
> gfxASurface::Release() is the #3 topcrash in 20.0a1 and 19.0a2 - see
> https://crash-stats.mozilla.com/report/
> list?signature=gfxASurface%3A%3ARelease%28%29&version=Firefox%3A19.0a2 - can
> we please get some progress here? Timothy? Bas?

I'm looking into this, I just haven't had anything to post yet.

Comment 33

[Scoobidiver (away)]

(In reply to Timothy Nikkel (:tn) from comment #28)
> The crash of comment 19 is almost certainly caused by bug 782980.
It's bug 793983.

(In reply to Robert Kaiser (:[e-mail]) from comment #29)
> gfxASurface::Release() is the #3 topcrash in 20.0a1 and 19.0a2
This crash is indeed a regression in 19.0.

Comment 34

[Scoobidiver (away)]

To clarify the scope of this bug, it occurs on startup and is a regression in 19.0 (first showed up around 19.0a1/20121025). It's #2 top browser crasher in 19.0a2 and #15 in 20.0a1.

Non-startup crashes with some signatures in common with this bug described in comment 10, comment 16, comment 19, and comment 28 are tracked in bug 793983.

Comment 35

[Timothy Nikkel (:tnikkel)]

I disagree, I'm going to track it this way, it'll work out for the best.

Comment 36

[u279076]

Timothy, Kairo asked that we get some QA on this bug. Is there some help that you need here? If so, ccing Matt Wobensmith to assist. Please advise, thank you.

Comment 37

[Timothy Nikkel (:tnikkel)]

I don't need any QA help, I'm in the process of writing the patch now.

Comment 38

[u279076]

(In reply to Timothy Nikkel (:tn) from comment #37)
> I don't need any QA help, I'm in the process of writing the patch now.

Okay, thank you Timothy and apologies if this was distracting.

Kairo, I assume this addresses your concerns. QA will be standing by for builds with Timothy's patch to test.

Comment 39

[Robert Kaiser]

I don't remember me saying we need QA here, just that we need progress here of some sort - but thanks, Anthony, for trying to get things going.
I'm even more delighted to hear about Timothy's progress, though, I'd surely be happy to get this one off the topcrash list!

That said, from a code POV, if this really affects 18 and is a sec-crit, we should request tracking and later approval for that train.

Comment 40

[Timothy Nikkel (:tnikkel)]

(In reply to Robert Kaiser (:[e-mail]) from comment #39)
> I don't remember me saying we need QA here, just that we need progress here
> of some sort - but thanks, Anthony, for trying to get things going.
> I'm even more delighted to hear about Timothy's progress, though, I'd surely
> be happy to get this one off the topcrash list!

Bug 782980 added a flush to the painting call path, which can destroy almost anything. Previously we would flush before painting on a WillPaint notification so the path should be mostly safe against this. I'm making it completely safe for this, which involves making the widget-backend specific painting code for each backend we have handle this case (destroying of |this|).

> That said, from a code POV, if this really affects 18 and is a sec-crit, we
> should request tracking and later approval for that train.

It would affect anywhere that bug 782980 landed, that bug has milestone 17, so 17, 18, 19, etc should be affected by that logic.

Comment 41

[Robert Kaiser]

Requesting tracking for 18 based on comment 40 saying this affects 17 and 18 and that it's marked sec-crit.
None of the crash signatures listed here are in the top 300 browser crashes on 18.0b3, though, while gfxASurface::Release() is still #2 on Aurora 19.

Comment 42

[Timothy Nikkel (:tnikkel)]

I think the nsView::DidPaintWindow crashes are probably caused by the same thing.

Comment 43

[Timothy Nikkel (:tnikkel)]

Attachment: Move the forced repaint from the Paint notification to the WillPaint notification.

Comment 44

[Timothy Nikkel (:tnikkel)]

Attachment: Re-get the view after calling PaintWindow because it can flush (via WillPaint).

Comment 45

[Timothy Nikkel (:tnikkel)]

Attachment: WillPaint notification can flush, so re-get the listener after it on GTK2 widget backend.

Karl, please double check that I don't miss doing something important in this function by adding an early return false.

Comment 46

[Timothy Nikkel (:tnikkel)]

Attachment: WillPaint notification can flush, so re-get the listener after it on Windows widget backend.

This is early enough in the function that there shouldn't be any state that we need to cleanup by my reading of this function.

Comment 47

[Timothy Nikkel (:tnikkel)]

Attachment: Paint notification can flush (via WillPaint), so re-check if the listener still exists after on puppet widget backend.

Comment 48

[Timothy Nikkel (:tnikkel)]

I checked that the major widget backends hold a reference to the widget during a paint event so we don't have to worry about it going away, just Destroy etc being called on it.

Comment 49

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 690659 [details] [diff] [review]
Paint notification can flush (via WillPaint), so re-check if the listener still exists after on puppet widget backend.

Nit, { }.

Comment 50

[Jim Mathies [:jimm]]

Comment on attachment 690657 [details] [diff] [review]
WillPaint notification can flush, so re-get the listener after it on Windows widget backend.

How about PaintWindow and DidPaintWindow? Do we need the same refresh there?

This seems a little brittle. Could listener come from a helper that always gets the current value from mAttachedWidgetListener or mWidgetListener and checks mDestroyCalled and then returns a result callers can check?

Comment 51

[Jim Mathies [:jimm]]

Attachment: winrt widget patch

Comment 52

[Timothy Nikkel (:tnikkel)]

(In reply to Jim Mathies [:jimm] from comment #50)
> How about PaintWindow and DidPaintWindow? Do we need the same refresh there?

Only WillPaintWindow should be allowed to flush.

> This seems a little brittle. Could listener come from a helper that always
> gets the current value from mAttachedWidgetListener or mWidgetListener and
> checks mDestroyCalled and then returns a result callers can check?

I can do that.

Comment 53

[Timothy Nikkel (:tnikkel)]

Comment on attachment 690817 [details] [diff] [review]
winrt widget patch

This looks fine, although I don't think you need to re-get the listener after PaintWindow. Add a comment why you are re-getting the listener after WillPaintWindow though?

Does the winrt code hold a reference to |this| during a call to MetroWidget::Paint? Because I think it should if it doesn't.

Comment 54

[Timothy Nikkel (:tnikkel)]

Attachment: WillPaint notification can flush, so re-get the listener after it on Windows widget backend.

Comment 55

[Timothy Nikkel (:tnikkel)]

Comment on attachment 690653 [details] [diff] [review]
Move the forced repaint from the Paint notification to the WillPaint notification.

I'm using this secapproval request for all patches in this bug.


[Security approval request comment]
How easily can the security issue be deduced from the patch?
The patches point out where destruction of objects can happen where we use them afterwards, but they don't obviously provide a way to hit that case. However there are STR posted in the open bug 793983. The check-in comments pretty much restate what is already added by in-code comments in the patches.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Same answer as above.

Which older supported branches are affected by this flaw?
version 17 and after

If not all supported branches, which bug introduced the flaw?
bug 782980

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
if the patches don't apply as is, backports should be straight forward.

How likely is this patch to cause regressions; how much testing does it need?
It should only be handling cases where we would crash before, so relatively safe.

Comment 56

[Jim Mathies [:jimm]]

(In reply to Timothy Nikkel (:tn) from comment #53)
> Comment on attachment 690817 [details] [diff] [review]
> winrt widget patch
> 
> This looks fine, although I don't think you need to re-get the listener
> after PaintWindow. Add a comment why you are re-getting the listener after
> WillPaintWindow though?
> 
> Does the winrt code hold a reference to |this| during a call to
> MetroWidget::Paint? Because I think it should if it doesn't.

Yes there are a few since there is only one main widget.

Comment 57

[Timothy Nikkel (:tnikkel)]

https://tbpl.mozilla.org/?tree=Try&rev=2dc6ec396b9f

Comment 58

[Alex Keybl [:akeybl]]

Please also prepare/nominate patches for branches.

Comment 59

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f6c579151c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/789e54325d76
https://hg.mozilla.org/integration/mozilla-inbound/rev/9aae7b226f3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/65d0aedb6ac4
https://hg.mozilla.org/integration/mozilla-inbound/rev/956f6e2b4b70

Comment 60

[Timothy Nikkel (:tnikkel)]

Not fixed until merged to m-c.

Comment 61

[Timothy Nikkel (:tnikkel)]

Jim, I'll leave landing the winrt patch to you as that code isn't merged to m-c/inbound yet.

Comment 62

[Bas Schouten (:bas.schouten)]

Is there anything we can do here to detect if something here goes wrong again? As far as I can tell this is still a pretty fragile balance, I'm worried we'll regress something like this again if we don't pay close attention.

Comment 63

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9f6c579151c5
https://hg.mozilla.org/mozilla-central/rev/789e54325d76
https://hg.mozilla.org/mozilla-central/rev/9aae7b226f3d
https://hg.mozilla.org/mozilla-central/rev/65d0aedb6ac4
https://hg.mozilla.org/mozilla-central/rev/956f6e2b4b70

Comment 64

[Timothy Nikkel (:tnikkel)]

Maybe we can add a test based on comment 19.

Comment 65

[Scoobidiver (away)]

64-bit crashes are not fixed: https://crash-stats.mozilla.com/report/list?signature=%400x0+|+nsBaseWidget%3A%3AAutoLayerManagerSetup%3A%3A~AutoLayerManagerSetup%28%29

Comment 66

[Timothy Nikkel (:tnikkel)]

Backed out "Move the forced repaint from the Paint notification to the WillPaint notification." because it made bug 813024 much worse
https://hg.mozilla.org/integration/mozilla-inbound/rev/62a769a3ae0e

Comment 67

[Timothy Nikkel (:tnikkel)]

Relanded because I landed a fix for bug 813024.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a268b5d3166d

Comment 68

[Timothy Nikkel (:tnikkel)]

We can open a new bug to deal with the crashes that remain, because we've clearly fixed some of the crashes.

Comment 69

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a268b5d3166d

Comment 70

[bhavana bajaj [:bajaj]]

(In reply to Timothy Nikkel (:tn) from comment #67)
> Relanded because I landed a fix for bug 813024.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/a268b5d3166d

This seems to have fixed top-crasher,Bug 793983.Can we have comment# 64 addressed and a patch uplifted for branches if it is low risk & well tested before tomorrow(beta 5 going to build) ?

Comment 71

[Timothy Nikkel (:tnikkel)]

(In reply to bhavana bajaj [:bajaj] from comment #70)
> (In reply to Timothy Nikkel (:tn) from comment #67)
> > Relanded because I landed a fix for bug 813024.
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/a268b5d3166d
> 
> This seems to have fixed top-crasher,Bug 793983.Can we have comment# 64
> addressed and a patch uplifted for branches if it is low risk & well tested
> before tomorrow(beta 5 going to build) ?

I'm not sure it will be well tested by tomorrow. The initial landing here made bug 813024 worse, and the fix for bug 813024 won't be in a nightly until tomorrow, we'd need to land bug 813024 as well as this bug on branches.

Comment 72

[Timothy Nikkel (:tnikkel)]

Comment on attachment 690653 [details] [diff] [review]
Move the forced repaint from the Paint notification to the WillPaint notification.

This is a request for all the patches.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 782980
User impact if declined: reproducible crashes
Testing completed (on m-c, etc.): been on nightly
Risk to taking this patch (and alternatives if risky): should just fix crashes, no real alternatives
String or UUID changes made by this patch: none

Comment 73

[bhavana bajaj [:bajaj]]

Comment on attachment 690653 [details] [diff] [review]
Move the forced repaint from the Paint notification to the WillPaint notification.

A relatively safe patch fixing a lot of crashes.Approving on branches.

Comment 74

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-beta/rev/254fe89b5c61
https://hg.mozilla.org/releases/mozilla-beta/rev/1a013ff8e15f
https://hg.mozilla.org/releases/mozilla-beta/rev/b0d963f152be
https://hg.mozilla.org/releases/mozilla-beta/rev/af287e869ef5

"Re-get the view after calling PaintWindow because it can flush (via WillPaint)." didn't need to be landed on beta as the problematic code doesn't exist there.

Comment 75

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6268ef939d15
https://hg.mozilla.org/releases/mozilla-aurora/rev/c987ef477b34
https://hg.mozilla.org/releases/mozilla-aurora/rev/572fcc921c2e
https://hg.mozilla.org/releases/mozilla-aurora/rev/eedd02509207
https://hg.mozilla.org/releases/mozilla-aurora/rev/a87c44ff3252

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/af287e869ef5
https://hg.mozilla.org/releases/mozilla-b2g18/rev/b0d963f152be
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1a013ff8e15f
https://hg.mozilla.org/releases/mozilla-b2g18/rev/254fe89b5c61

Comment 77

[u279076]

Considering this was only reproducible in optimized builds, would it be possible to get optimized builds generated for each of the branches where this is fixed for verification?

Comment 78

[Timothy Nikkel (:tnikkel)]

Optimized builds are what we ship to users, so they should be easily available.

Comment 79

[u279076]

(In reply to Timothy Nikkel (:tn) from comment #78)
> Optimized builds are what we ship to users, so they should be easily
> available.

Okay, thanks. I'll make an attempt at reproducing and verifying this fixed after my meetings today.

Comment 80

[u279076]

Crash reproduced using a slight revision of the steps in comment 19 (Farcry 3 is no longer preorder):

1. Load http://shop.ubi.com
2. Select "Far Cry" from "Our Brands" menu
3. Select "Add to Cart" from "Far Cry 3"
4. Select "Add to Cart" from "PC Download"
5. Select "Checkout"
6. Click the down arrow on one of the dropdown boxes for the "Billing Information"
> Crash after a couple second hang

Reproduced in 2012-11-27 Firefox Nightly. Will now try to verify fixed.

Comment 81

[u279076]

Verified fixed in:
 * Firefox 20.0a1 2012-12-19
 * Firefox 19.0a2 2012-12-19
 * Firefox 18.0b5 build 1

Comment 82

[u279076]

I'm wondering if it would be useful to come up with a minimized testcase so we can get this checked into the testsuite. If so, I can get someone in QA to assist with that.

Comment 83

[Timothy Nikkel (:tnikkel)]

I'm not sure if this is sufficient to generate a crash but the important pieces of the ubi testcase where a select that has a onfocus handler that caused a frame reconstruct, so for example changing the CSS display of the select between block and inline would do it.

Comment 84

[u279076]

(In reply to Timothy Nikkel (:tn) from comment #83)
> I'm not sure if this is sufficient to generate a crash but the important
> pieces of the ubi testcase where a select that has a onfocus handler that
> caused a frame reconstruct, so for example changing the CSS display of the
> select between block and inline would do it.

What would you need to have an in-testsuite testcase? Isolate the HTML/CSS/JS into a mochitest?

Comment 85

[Timothy Nikkel (:tnikkel)]

I would just construct a page meeting the description of comment 83 and see if it reproduces the problem. If not I would try reducing the ubi testcase. And then turn that into an automated test by putting it into mochitest format and automating the clicking to cause the crash.

Comment 86

[u279076]

(In reply to Timothy Nikkel (:tn) from comment #85)
> I would just construct a page meeting the description of comment 83 and see
> if it reproduces the problem. If not I would try reducing the ubi testcase.
> And then turn that into an automated test by putting it into mochitest
> format and automating the clicking to cause the crash.

Okay, thank you Timothy. I'm ccing Kamil on this bug to fulfill that request. Kamil is one of our trusted community security testers.

Kamil, as a first step, can you try to mock up and attach a minimized HTML test based on comment 83 which reproduces this crash. If unsuccessful, try creating a minimized HTML test for the shop.ubi.com test outlined in comment 81. Thanks

Comment 87

[Timothy Nikkel (:tnikkel)]

Comment on attachment 690653 [details] [diff] [review]
Move the forced repaint from the Paint notification to the WillPaint notification.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: it's listed as sec-crit
User impact if declined: some amount of crashes
Fix Landed on Version: 18, 19, 20
Risk to taking this patch (and alternatives if risky): not very risky
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 88

[Kamil Jozwiak [:kjozwiak]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #86)
> Kamil, as a first step, can you try to mock up and attach a minimized HTML
> test based on comment 83 which reproduces this crash.

Yup will do!

Comment 89

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/d3f6ea42bf2b
https://hg.mozilla.org/releases/mozilla-esr17/rev/835cfd3111b6
https://hg.mozilla.org/releases/mozilla-esr17/rev/4fa6d2e7f89d
https://hg.mozilla.org/releases/mozilla-esr17/rev/02abf9bf846b

(I went with the same set of patches that landed on beta)

Comment 90

[Kamil Jozwiak [:kjozwiak]]

Attachment: Reproduce Crash using Ubi Case

Went through the Ubi case that was mentioned above and narrowed it down to a single drop down. Reproduced the problem several times using the following build:

http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2012/11/2012-11-27-04-20-13-mozilla-aurora/firefox-19.0a2.en-US.win32.installer.exe

Comment 91

[u279076]

Thanks for the minimized test, Kamil.

Timothy or Ryan, is the attached testcase enough to generate an in-testsuite test for this bug? If so, can one of you mentor Kamil in writing a patch?

Comment 92

[Timothy Nikkel (:tnikkel)]

It looks good, but hopefully we can get rid jquery from the testcase, it's a lot of extra stuff we don't need. I hope that if we just rewrite the bit of inline javascript at the bottom to work without jquery that will be enough.

Comment 93

[u279076]

(In reply to Timothy Nikkel (:tn) from comment #92)
> It looks good, but hopefully we can get rid jquery from the testcase, it's a
> lot of extra stuff we don't need. I hope that if we just rewrite the bit of
> inline javascript at the bottom to work without jquery that will be enough.

Kamil, can you take a crack at this?

Comment 94

[Kamil Jozwiak [:kjozwiak]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #93)
> (In reply to Timothy Nikkel (:tn) from comment #92)
> > It looks good, but hopefully we can get rid jquery from the testcase, it's a
> > lot of extra stuff we don't need. I hope that if we just rewrite the bit of
> > inline javascript at the bottom to work without jquery that will be enough.
> 
> Kamil, can you take a crack at this?

Yup, I will try removing the jquery. I took a look at it but it was hard to go through as there's a lot. Will try narrowing it down without the jquery.

Comment 95

[Kamil Jozwiak [:kjozwiak]]

Attachment: reduced test case without jquery

Went through the test case and completely removed jquery leaving only plain javascript. The file has been reduced to 432 bytes from 57,997 bytes.

The crash happens when the style is set to focus and the iframe having a height of 1.

Let me know if this works!

Comment 96

[Timothy Nikkel (:tnikkel)]

That looks great, thanks!

Do you want someone to help you turning it into a mochitest or do you want someone else to do that part?

Comment 97

[Kamil Jozwiak [:kjozwiak]]

(In reply to Timothy Nikkel (:tn) from comment #96)
> That looks great, thanks!
> 
> Do you want someone to help you turning it into a mochitest or do you want
> someone else to do that part?

I am up for it! Might take a little longer then usual as I've never built the code but I am definitely up for it. I'll read up on the MDN docs and get back to you if I have trouble.

Comment 98

[Timothy Nikkel (:tnikkel)]

Sure. It might help to look at an example. Here is a relatively simple mochitest that you can use as a template http://mxr.mozilla.org/mozilla-central/source/content/events/test/test_bug447736.html?force=1

To send a mouse event to open the dropdown you could use the synthesizeMouse function from EventUtils.js. There are numerous examples in the tree of it being used.

Comment 99

[Kamil Jozwiak [:kjozwiak]]

Attachment: mochitest

I created a test that does the same as the simplified test case.
It synthesizes mouse events to the drop down so that it expands.
This makes the focus event fire and the style is changed which usually causes a crash.

I checked out the code for an old aurora build so I could see if the manual test crashes.
But I was having trouble reproducing the crash even with that older checked out code, when it is run from the objdir.
But when I install it, it reproduces fine.

When I run the newly created automated test, even know it does the same as what I do manually, it doesn't crash.
So I'm not sure if you want to:
1) Add the test because maybe it would crash on the test servers if this bug still existed
2) Make a suggestion on how I can reproduce it with the test.
or 3) Not take this patch since I can't get it to crash.

If you do want to go ahead with the patch, please let me know if you'd like me to post this in a new bug instead since this bug is already resolved.

Comment 100

[Timothy Nikkel (:tnikkel)]

Comment on attachment 706939 [details] [diff] [review]
mochitest

This looks good, thank you!

We should just check that it does fail in an unpatched build. I can do that.