crash in nsWindow::OnPaint

VERIFIED FIXED in Firefox 18

Status

()

defect
--
critical
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: scoobidiver, Assigned: tnikkel)

Tracking

(Blocks 1 bug, 5 keywords)

19 Branch
mozilla20
All
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox17 wontfix, firefox18+ verified, firefox19+ verified, firefox20+ verified, firefox-esr10 unaffected, firefox-esr1718+ fixed, b2g18 fixed)

Details

(Whiteboard: [startupcrash][adv-main18+][adv-esr17+], crash signature)

Attachments

(9 attachments, 1 obsolete attachment)

2.26 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.17 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
1.10 KB, patch
karlt
: review+
Details | Diff | Splinter Review
1017 bytes, patch
cjones
: review+
Details | Diff | Splinter Review
2.17 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
2.97 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
56.64 KB, text/html
Details
432 bytes, text/html
Details
2.31 KB, patch
kjozwiak
: review?
tnikkel
Details | Diff | Splinter Review
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
Crash Signature: [@ mozilla::layers::ImageContainer::Release()] → [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()]
Hardware: x86 → All
Crash Signature: [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] → [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ nsWindow::OnPaint(HDC__*, unsigned int)]
It might be a regression from bug 782547.
Crash Signature: [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ nsWindow::OnPaint(HDC__*, unsigned int)] → [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ nsWindow::OnPaint(HDC__*, unsigned int)] [@ gfxContext::~gfxContext()]
Component: Graphics: Layers → Widget: Win32
Summary: crash in mozilla::layers::ImageContainer::Release → crash in nsWindow::OnPaint
Crash Signature: [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ nsWindow::OnPaint(HDC__*, unsigned int)] [@ gfxContext::~gfxContext()] → [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ nsWindow::OnPaint(HDC__*, unsigned int)] [@ gfxContext::~gfxContext()] [@ _moz_cairo_destroy]
With combined signatures, it's #3 top crasher over the last three days.
Crash Signature: [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ nsWindow::OnPaint(HDC__*, unsigned int)] [@ gfxContext::~gfxContext()] [@ _moz_cairo_destroy] → [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ nsWindow::OnPaint(HDC__*, unsigned int)] [@ gfxContext::~gfxContext()] [@ _moz_cairo_destroy] [@ gfxASurface::Release()]
Crash Signature: [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ nsWindow::OnPaint(HDC__*, unsigned int)] [@ gfxContext::~gfxContext()] [@ _moz_cairo_destroy] [@ gfxASurface::Release()] → [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ nsWindow::OnPaint(HDC__* unsigned int)] [@ gfxContext::~gfxContext()] [@ _moz_cairo_destroy] [@ gfxASurface::Release()] [@ mozilla::l…
(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.
(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
gfxASurface::Release() saw a significant increase on 2012-10-31, we really need to get some traction on those issues.
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.
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.
Assignee: nobody → enndeakin
Keywords: needURLs, qawanted
The patch in bug 782547 was only in from around Oct 24 and was backed out on Oct 27.
Keywords: needURLs
QA Contact: mozillamarcia.knous
Crash Signature: [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ nsWindow::OnPaint(HDC__* → [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ @0x0 | nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ nsWindow::OnPaint(HDC__*
Crash Signature: [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ @0x0 | nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ nsWindow::OnPaint(HDC__* → [@ mozilla::layers::ImageContainer::Release()] [@ nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup()] [@ @0x0 | nsBaseWidget::AutoLayerManagerSetup::~AutoLayerManagerSetup() ] [@ nsWindow::OnPaint(HDC__*
Assignee: enndeakin → nobody
Crash Signature: unsigned int)] [@ gfxContext::~gfxContext()] [@ _moz_cairo_destroy] [@ gfxASurface::Release()] [@ mozilla::layers::Layer::~Layer()] → unsigned int)] [@ gfxContext::~gfxContext()] [@ _moz_cairo_destroy] [@ gfxASurface::Release()] [@ mozilla::layers::Layer::~Layer()] [@ nsTArray<mozilla::css::ComputedTimingFunction*, nsTArrayInfallibleAllocator>::Clear() | nsTArray<void* nsTArrayInfa…
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.
(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?
I doubt that this is a 10 month old crash. Maybe 793983.
Marcia, have you been able to reproduce this crash?
Assignee: nobody → bas
I can reproduce this on Fx17 but -not- on a debug build of nightly. I'll look into this a little more.
I strongly suspect this might be related to bug 743975
Bas, can you provide more details about how you reproduce this issue?
(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.
I'm marking this security sensitive. Putting more details in a restricted comment.
Group: core-security
Comment 21 is private: false
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.
Assignee: bas → 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.
Blocks: 782980
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?
(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?
No, I don't think we need any qa help with this right now.
(In reply to Robert Kaiser (:kairo@mozilla.com) 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.
(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 (:kairo@mozilla.com) from comment #29)
> gfxASurface::Release() is the #3 topcrash in 20.0a1 and 19.0a2
This crash is indeed a regression in 19.0.
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.
I disagree, I'm going to track it this way, it'll work out for the best.
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.
Keywords: qawanted
QA Contact: mozillamarcia.knous → mwobensmith
I don't need any QA help, I'm in the process of writing the patch now.
(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.
Keywords: qawanted
Blocks: 793983
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.
(In reply to Robert Kaiser (:kairo@mozilla.com) 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.
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.
I think the nsView::DidPaintWindow crashes are probably caused by the same thing.
Karl, please double check that I don't miss doing something important in this function by adding an early return false.
Attachment #690655 - Flags: review?(karlt)
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.
Attachment #690657 - Flags: review?(jmathies)
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.
Attachment #690653 - Flags: review?(matt.woodrow) → review+
Attachment #690654 - Flags: review?(matt.woodrow) → review+
Attachment #690655 - Flags: review?(karlt) → review+
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, { }.
Attachment #690659 - Flags: review?(jones.chris.g) → review+
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?
Attachment #690657 - Flags: review?(jmathies) → review+
Attachment #690817 - Flags: review?(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 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.
Attachment #690817 - Flags: review?(tnikkel) → review+
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.
Attachment #690653 - Flags: sec-approval?
(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.
Attachment #690653 - Flags: sec-approval? → sec-approval+
Please also prepare/nominate patches for branches.
Not fixed until merged to m-c.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Jim, I'll leave landing the winrt patch to you as that code isn't merged to m-c/inbound yet.
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.
Maybe we can add a test based on comment 19.
No longer blocks: 813024
Depends on: 813024
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
We can open a new bug to deal with the crashes that remain, because we've clearly fixed some of the crashes.
(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) ?
(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 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
Attachment #690653 - Flags: approval-mozilla-beta?
Attachment #690653 - Flags: approval-mozilla-aurora?
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.
Attachment #690653 - Flags: approval-mozilla-beta?
Attachment #690653 - Flags: approval-mozilla-beta+
Attachment #690653 - Flags: approval-mozilla-aurora?
Attachment #690653 - Flags: approval-mozilla-aurora+
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?
Optimized builds are what we ship to users, so they should be easily available.
(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.
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.
Verified fixed in:
 * Firefox 20.0a1 2012-12-19
 * Firefox 19.0a2 2012-12-19
 * Firefox 18.0b5 build 1
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.
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.
(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?
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.
(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
Blocks: 822999
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.
Attachment #690653 - Flags: approval-mozilla-esr17?
(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!
Attachment #690653 - Flags: approval-mozilla-esr17? → approval-mozilla-esr17+
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
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?
Flags: in-testsuite?
Keywords: testcase
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.
(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?
(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.
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!
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?
(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.
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.
Whiteboard: [startupcrash] → [startupcrash][adv-main18+][adv-esr17+]
Posted patch mochitestSplinter Review
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.
Attachment #706939 - Flags: review?(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.
Group: core-security
You need to log in before you can comment on or make changes to this bug.