154566 - live skin switch [@nsBox::GetBorder]

Status: RESOLVED INACTIVE

(Core :: XUL, defect)

Description

[timeless]

TB7756597W

#1 http://lxr.mozilla.org/seamonkey/source/content/base/src/nsRuleNode.cpp#4743
nsRuleNode::GetStyleData can return null
#2 so the only other root flavor of getstyle data is here
3953 bzbarsky   1.34   NS_ASSERTION(aStyleStruct, "Failed to get a style 
struct");
3954 harishd    1.14   return NS_OK;
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/style/src/nsCo
mputedDOMStyle.cpp#3921
<bz> well... we can get null structs in computed style certainly...
 * bz seems to recall getting those...
<bz> not sure how, though

 * timeless concludes that disp is defintely likely to be null
however, i'd argue that perhaps returning NS_OK is wrong :)

#3: there are a bunch of GetStyleDatas but they call other GetStyleData's
they should eventually hit #1 or #2, both of which can return null
#4
 661                   frame->GetStyleData(eStyleStruct_Display,
 662                                       (const nsStyleStruct*&) disp);
 663 /*crash here */   if (disp->mAppearance && gTheme) {

the guess is that disp was null, how's my driving?

<bz> heh
see also bug 117088

nsBox::GetBorder
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBox.cpp, line 663]
nsBox::GetBorderAndPadding
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBox.cpp, line 637]
nsBox::GetClientRect
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBox.cpp, line 537]
nsSprocketLayout::Layout
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsSprocketLayout.cpp, line 
228]
nsContainerBox::DoLayout
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsContainerBox.cpp, line 607]
nsBox::Layout
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBox.cpp, line 1062]
nsMenuFrame::DoLayout
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsMenuFrame.cpp, line 1067]
nsBox::Layout
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBox.cpp, line 1062]
nsBoxFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 1002]
nsLineLayout::ReflowFrame
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsLineLayout.cpp, line 1100]
nsInlineFrame::ReflowInlineFrame
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsInlineFrame.cpp, line 718]
nsInlineFrame::ReflowFrames
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsInlineFrame.cpp, line 523]
nsInlineFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsInlineFrame.cpp, line 437]
nsLineLayout::ReflowFrame
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsLineLayout.cpp, line 1100]
nsBlockFrame::ReflowInlineFrame
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 3800]
nsBlockFrame::DoReflowInlineFrames
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 3626]
nsBlockFrame::DoReflowInlineFramesAuto
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 3516]
nsBlockFrame::ReflowInlineFrames
[c:/builds/seamonkey/mozilla/lay out/html/base/src/nsBlockFrame.cpp, line 3461]
nsBlockFrame::ReflowLine
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 2610]
nsBlockFrame::ReflowDirtyLines
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 2254]
nsBlockFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 951]
nsContainerFrame::ReflowChild
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 
825]
nsTableCellFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableCellFrame.cpp, line 
946]
nsContainerFrame::ReflowChild
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 
825]
nsTableRowFrame::ReflowChildren
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableRowFrame.cpp, line 
1043]
nsTableRowFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableRowFrame.cpp, line 
1461]
nsContainerFrame::ReflowChild
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 
825]
nsTableRowGroupFrame::ReflowChildren
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableRowGroupFrame.cpp, 
line 447]
nsTableRowGroupFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableRowGroupFrame.cpp, 
line 1217]
nsContainerFrame::ReflowChild
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 
825]
nsTableFrame::ReflowChildren
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableFrame.cpp, line 3313]
nsTableFrame::ReflowTable
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableFrame.cpp, line 2207]
nsTableFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableFrame.cpp, line 2073]
nsContainerFrame::ReflowChild
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 
825]
nsTableOuterFrame::OuterReflowChild
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableOuterFrame.cpp, line 
1027]
nsTableOuterFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableOuterFrame.cpp, line 
1612]
nsBlockReflowContext::DoReflowBlock
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockReflowContext.cpp, 
line 570]
nsBlockReflowContext::ReflowBlock
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockReflowContext.cpp, 
line 348]
nsBlockFrame::ReflowBlockFrame
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 3222]
nsBlockFrame::ReflowLine
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 2476]
nsBlockFrame::ReflowDirtyLines
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 2254]
nsBlockFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 951]
nsContainerFrame::ReflowChild
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 
825]
nsTableCellFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableCellFrame.cpp, line 
946]
nsContainerFrame::ReflowChild
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 
825]
nsTableRowFrame::ReflowChildren
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableRowFrame.cpp, line 
1043]
nsTableRowFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableRowFrame.cpp, line 
1461]
nsContainerFrame::ReflowChild
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 
825]
nsTableRowGroupFrame::ReflowChildren
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableRowGroupFrame.cpp, 
line 447]
nsTableRowGroupFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableRowGroupFrame.cpp, 
line 1217]
nsContainerFrame::ReflowChild
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 
825]
nsTableFrame::ReflowChildren
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableFrame.cpp, line 3313]
nsTableFrame::ReflowTable
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableFrame.cpp, line 2207]
nsTableFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableFrame.cpp, line 2073]
nsContainerFrame::ReflowChild
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsContainerFrame.cpp, line 
825]
nsTableOuterFrame::OuterReflowChild
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableOuterFrame.cpp, line 
1027]
nsTableOuterFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/table/src/nsTableOuterFrame.cpp, line 
1612]
nsBlockReflowContext::DoReflowBlock
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockReflowContext.cpp, 
line 570]
nsBlockReflowContext::ReflowBlock
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockReflowContext.cpp, 
line 348]
nsBlockFrame::ReflowBlockFrame
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 3222]
nsBlockFrame::ReflowLine
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 2476]
nsBlockFrame::ReflowDirtyLines
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 2254]
nsBlockFrame::Reflow
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockFrame.cpp, line 951]
nsBlockReflowContext::DoReflowBlock
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsBlockReflowContext.cpp, 
line 570]

quick research indicates that disp is probably null, and that it's not 
impossible by code flow for this to happen.

Comment 1

[timeless]

Attachment: check for null in nsComputedDOMStyle.cpp, GetStyleData's out var is always checked for null

in the last fragment of this patch (where i'm just twiddling the rv decl), we
could crash unless the function consistently returns a failure for null out. So
instead of adding a null check here, i put in the ensure true...

Comment 2

[Jan Varga [:janv]]

Comment on attachment 89388 [details] [diff] [review]
check for null in nsComputedDOMStyle.cpp, GetStyleData's out var is always checked for null

r=varga, but please test it well

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 89388 [details] [diff] [review]
check for null in nsComputedDOMStyle.cpp, GetStyleData's out var is always checked for null

You shouldn't need to null-check these results unless something has gone very
dramatically wrong.  Do you have a testcase for reproducing the crash?

Also, nsIFrame::GetStyleData always returns NS_OK, so there's no point
returning its style data.

If we really need null-checks for GetStyleData, we should put them *inside*
GetStyleData and have it return dummy data if something fails.	We shouldn't
null check at the call sites.

Comment 4

[timeless]

i had trouble interpretting dbaron's comment but caillon suggested that he 
meant that we should fix GetStyleData to never return null, but out a new 
nsStyleFoo object with nothing init'd.

drbrain triggered the crash, but basically he ran with live locale/skin 
switching and presumably tried to switch skins.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

How do I run with live locale/skin switching?

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Here's a reply timeless sent me by email.  In the future, please reply on the
bug.  Also, note that the URL field is traditionally used for testcases and
people don't always look there for a URL that has some other random relationship
to the bug, especially if it's not mentioned in the bug comments.


Date: Thu, 27 Jun 2002 19:59:19 -0400
Subject: Re: [Bug 154566] live skin switch  [@nsBox::GetBorder]
From: timeless <[e-mail]>
To: [e-mail]

http://bugzilla.mozilla.org/show_bug.cgi?id=154566

Comments From [e-mail]  2002-06-27 16:50
> How do I run with live locale/skin switching?

apply the patch in the url,
use on of
view>apply theme
edit>preferences>appearance>content packs
edit>preferences>navigator>skins

Comment 7

[Carlos Alén Silva]

14 years later... Is this still an issue?

Comment 8

[Timea Cernea [:tbabos][inactive]]

Hey Timeless,
Are you still considering working on this given you are the Assignee for this issue? Is it still reproducible? If not, please consider closing this issue. Thanks!

Comment 9

[timeless]

Most live switching was abandoned a long time ago. I'm fairly certain most of the code was reworked between then and now.

No, keeping bugs like this open aren't particularly useful, and no, asking me about them isn't either.