315483 - crash on getComputedStyle getting certain properties on an element that's display:none [@ nsHTMLReflowState::GetContainingBlockFor]

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Description

[Adrian Yee]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051107 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051107 Firefox/1.5

Here's the situation:
- a div that has two styles: display: none, and padding: 10%
- javascript that uses getComputedStyle to return 'padding-left' will crash the browser

If you attempt to get the property 'padding', it doesn't crash (shown by the "what's the padding?" link).  Also, if the div is display:block, then the crash doesn't occur.

Reproducible: Always

Steps to Reproduce:
1. Visit testcase url
2. Click on the "What's the left padding?" link

Actual Results:  
The browser crashes.

Expected Results:  
A javascript alert with the padding value should be shown.

Comment 1

[Adrian Yee]

I forgot to add that if the padding is changed to a different unit (eg. px), it doesn't crash.

Comment 2

[Gérard Talbot]

I crashed with Seamonkey 1.5a rv:1.9a1 build 2005110609 under XP Pro SP2:
Talkback id: TB11576709X

I also crashed with Firefox 1.5 rv:1.8 build 20051107:
Talkback id: TB11576836Y

Adding talkbackid keyword

I have not checked for duplicate yet.

Comment 3

[timeless]

Incident ID: 11576836
Stack Signature	nsHTMLReflowState::GetContainingBlockFor 717b16ce
Product ID	Firefox15
Build ID	2005110708
Trigger Time	2005-11-07 18:29:28.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	firefox.exe + (00207970)
URL visited	
User Comments	Bug 315483
Since Last Crash	7150 sec
Total Uptime	7150 sec
Trigger Reason	Access violation
Source File, Line No.	c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsHTMLReflowState.cpp, line 457
Stack Trace 	
nsHTMLReflowState::GetContainingBlockFor  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/generic/nsHTMLReflowState.cpp, line 457]
CalcSidesFor  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/style/nsStyleStruct.cpp, line 132]
nsStylePadding::CalcPaddingFor  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/style/nsStyleStruct.cpp, line 419]
nsComputedDOMStyle::GetPaddingWidthFor  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/style/nsComputedDOMStyle.cpp, line 3326]
nsComputedDOMStyle::GetPropertyValue  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/layout/style/nsComputedDOMStyle.cpp, line 280]
XPTC_InvokeByIndex  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp, line 102]
XPCWrappedNative::CallMethod  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2139]
XPC_WN_CallMethod  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp, line 1444]
js_Invoke  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1177]
js_Interpret  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 3523]
js_Execute  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsinterp.c, line 1424]
JS_EvaluateUCScriptForPrincipals  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/js/src/jsapi.c, line 4103]
nsJSContext::EvaluateString  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1061]
nsJSThunk::EvaluateScript  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp, line 297]
nsJSChannel::InternalOpen  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp, line 550]
nsJSChannel::AsyncOpen  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/dom/src/jsurl/nsJSProtocolHandler.cpp, line 522]
nsURILoader::OpenURI  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/uriloader/base/nsURILoader.cpp, line 915]
nsDocShell::DoChannelLoad  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/docshell/base/nsDocShell.cpp, line 6953]
nsDocShell::DoURILoad  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/docshell/base/nsDocShell.cpp, line 6811]
nsDocShell::InternalLoad  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/docshell/base/nsDocShell.cpp, line 6578]
nsWebShell::OnLinkClickSync  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/docshell/base/nsWebShell.cpp, line 549]
OnLinkClickEvent::HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/docshell/base/nsWebShell.cpp, line 345]
PL_HandleEvent  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/xpcom/threads/plevent.c, line 689]
0x778b0c24
nsFormControlList::RemoveElementFromTable  [c:/builds/tinderbox/Fx-Mozilla1.8/WINNT_5.2_Depend/mozilla/content/html/content/src/nsHTMLFormElement.cpp, line 1994]
0xffffff74

Comment 4

[Adam Guthrie]

The stack I get from the testcase is very similar to the one in bug 273458 attachment 168555 [details] (which is supposedly fixed). Might be a dupe of bug 194952.

Comment 5

[jpl24]

Attachment: patch_v1

Simple one-liner to fix the crash. This was the only place where GetContainingBlockFor was called without checking for a null frame first.

Also, I agree with the XXX comment. That function shouldn't be inline.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 228539 [details] [diff] [review]
patch_v1

I guess this is good enough, yeah.  r+sr=bzbarsky.

Do you need this checked in?

Comment 7

[jpl24]

If you could check this in that'd be great. Thanks.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 9

[Mike Connor [:mconnor]]

Can we get this on the branch ASAP?

Comment 10

[Martijn Wargers (dead)]

Checking in nsStyleStruct.cpp;
/cvsroot/mozilla/layout/style/nsStyleStruct.cpp,v  <--  nsStyleStruct.cpp
new revision: 3.129.6.3; previous revision: 3.129.6.2
done

Checked into 1.8.1 branch.