236921 - [FIXr]CRASH when inspecting div with border-color inherit

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect, P2)

Description

[Stephen Oberholtzer]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040219
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7a) Gecko/20040219

When using the DOM inspector to inspect an element with style 'display: none'
and 'border-color: inherit', attempting to view 'computed style' crashes with
what looks like a null object dereference (invalid memory address 0x0000001c).

Reproducible: Always
Steps to Reproduce:
1. Load the attached HTML page in Mozilla.
2. Navigate DOM inspector to the div whose text is 'this is the inner div'. 
Note: This is a LOT easier if you have the 'InspectThis' extension installed.
3. Once the proper div is selected, attempt to change the view from 'DOM Node'
to 'Computed Style'.

Actual Results:  
*boom*

Expected Results:  
I expected to see the computed style rules for the selected div :)

Comment 1

[Stephen Oberholtzer]

Attachment: Example page that demonstrates the bug

Comment 2

[Rene Pronk]

Confirming with Mozilla 1.7a under WinXP.

I get exactly the same invalid memory address, almost sounds like pointer
problem or something like that.

It crashes right here:

http://lxr.mozilla.org/seamonkey/source/content/shared/public/nsRuleNode.h#211

205     const StyleStructInfo& info = gInfo[aSID];
206 
207     // Get either &mInheritedData or &mResetData.
208     char* resetOrInheritSlot = NS_REINTERPRET_CAST(char*, this) +
info.mCachedStyleDataOffset;
209 
210     // Get either mInheritedData or mResetData.
211     char* resetOrInherit = NS_REINTERPRET_CAST(char*,
*NS_REINTERPRET_CAST(void**, resetOrInheritSlot));
212 


Here's the stacktrace:

nsCachedStyleData::GetStyleData(const nsStyleStructID & eStyleStruct_Color) line
211 + 3 bytes
nsStyleContext::GetStyleData(nsStyleStructID eStyleStruct_Color) line 247 + 15 bytes
nsStyleContext::GetStyleColor() line 70 + 17 bytes
nsRuleNode::ComputeBorderData(nsStyleStruct * 0x03bacd44, const nsCSSStruct &
{...}, nsStyleContext * 0x03bad9f4, nsRuleNode * 0x03bad614, const
nsRuleNode::RuleDetail & eRulePartialMixed, int 0) line 3239 + 8 bytes
nsRuleNode::WalkRuleTree(nsStyleStructID eStyleStruct_Border, nsStyleContext *
0x03bad9f4, nsRuleData * 0x0012d63c, nsCSSStruct * 0x0012d67c) line 129 + 42 bytes
nsRuleNode::GetBorderData(nsStyleContext * 0x03bad9f4) line 1095 + 31 bytes
nsRuleNode::GetStyleData(nsStyleStructID eStyleStruct_Border, nsStyleContext *
0x03bad9f4, int 1) line 129 + 12 bytes
nsStyleContext::GetStyleData(nsStyleStructID eStyleStruct_Border) line 251
nsComputedDOMStyle::GetStyleData(nsStyleStructID eStyleStruct_Border, const
nsStyleStruct * & 0x00000000, nsIFrame * 0x00000000) line 2924 + 12 bytes
nsComputedDOMStyle::GetBorderColorFor(unsigned char 2, nsIFrame * 0x00000000,
nsIDOMCSSValue * * 0x0012d924) line 3237
nsComputedDOMStyle::GetBorderBottomColor(nsIFrame * 0x00000000, nsIDOMCSSValue *
* 0x0012d924) line 1074
nsComputedDOMStyle::GetPropertyCSSValue(nsComputedDOMStyle * const 0x03bd3070,
const nsAString & {...}, nsIDOMCSSValue * * 0x0012d924) line 285 + 21 bytes
nsComputedDOMStyle::GetPropertyValue(nsComputedDOMStyle * const 0x03bd3070,
const nsAString & {...}, nsAString & {...}) line 250 + 40 bytes
XPTC_InvokeByIndex(nsISupports * 0x03bd3070, unsigned int 5, unsigned int 2,
nsXPTCVariant * 0x0012daa4) line 102
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_METHOD) line 2022 + 42 bytes
XPC_WN_CallMethod(JSContext * 0x02022670, JSObject * 0x032845f0, unsigned int 1,
long * 0x031a17f8, long * 0x0012dd74) line 1287 + 14 bytes
js_Invoke(JSContext * 0x02022670, unsigned int 1, unsigned int 0) line 941 + 23
bytes
js_Interpret(JSContext * 0x02022670, long * 0x0012e6a8) line 2962 + 15 bytes
js_Invoke(JSContext * 0x02022670, unsigned int 2, unsigned int 2) line 958 + 13
bytes
nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJSClass * const 0x02a13f68,
nsXPCWrappedJS * 0x03a6a870, unsigned short 23, const nsXPTMethodInfo *
0x02057f68, nsXPTCMiniVariant * 0x0012e9f4) line 1336 + 22 bytes
nsXPCWrappedJS::CallMethod(nsXPCWrappedJS * const 0x03a6a870, unsigned short 23,
const nsXPTMethodInfo * 0x02057f68, nsXPTCMiniVariant * 0x0012e9f4) line 450
PrepareAndDispatch(nsXPTCStubBase * 0x03a6a870, unsigned int 23, unsigned int *
0x0012eaa4, unsigned int * 0x0012ea94) line 117 + 31 bytes
SharedStub() line 147
nsTreeBodyFrame::PaintText(int 4, nsTreeColumn * 0x03b41430, const nsRect &
{...}, nsIPresContext * 0x0310f368, nsIRenderingContext & {...}, const nsRect &
{...}, int & 3757) line 2809
nsTreeBodyFrame::PaintCell(int 4, nsTreeColumn * 0x03b41430, const nsRect &
{...}, nsIPresContext * 0x0310f368, nsIRenderingContext & {...}, const nsRect &
{...}, int & 1500) line 2575
nsTreeBodyFrame::PaintRow(int 4, const nsRect & {...}, nsIPresContext *
0x0310f368, nsIRenderingContext & {...}, const nsRect & {...}) line 2417
nsTreeBodyFrame::Paint(nsTreeBodyFrame * const 0x03198058, nsIPresContext *
0x0310f368, nsIRenderingContext & {...}, const nsRect & {...}, nsFramePaintLayer
eFramePaintLayer_Overlay, unsigned int 0) line 2202
PresShell::Paint(PresShell * const 0x03a48454, nsIView * 0x03b3cbd0,
nsIRenderingContext & {...}, const nsRect & {...}) line 5621 + 31 bytes
nsView::Paint(nsView * const 0x03b3cbd0, nsIRenderingContext & {...}, const
nsRect & {...}, unsigned int 0, int & 0) line 262
nsViewManager::RenderDisplayListElement(DisplayListElement2 * 0x03156888,
nsIRenderingContext * 0x03b64170) line 1399
nsViewManager::RenderViews(nsView * 0x03b3cbd0, nsIRenderingContext & {...},
const nsRegion & {...}, void * 0x032c46e0) line 1317
nsViewManager::Refresh(nsView * 0x03b3cbd0, nsIRenderingContext * 0x03b64170,
nsIRegion * 0x03abf7a8, unsigned int 1) line 882
nsViewManager::DispatchEvent(nsViewManager * const 0x03aa5e60, nsGUIEvent *
0x0012f678, nsEventStatus * 0x0012f584) line 1867
HandleEvent(nsGUIEvent * 0x0012f678) line 79
nsWindow::DispatchEvent(nsWindow * const 0x03b3cc6c, nsGUIEvent * 0x0012f678,
nsEventStatus & nsEventStatus_eIgnore) line 1064 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f678, nsEventStatus &
nsEventStatus_eIgnore) line 1090
nsWindow::OnPaint(HDC__ * 0x00000000) line 5000 + 28 bytes
nsWindow::ProcessMessage(unsigned int 15, unsigned int 0, long 0, long *
0x0012fb60) line 3780 + 19 bytes
nsWindow::WindowProc(HWND__ * 0x001103fe, unsigned int 15, unsigned int 0, long
0) line 1346 + 27 bytes
USER32! 77d43a50()
USER32! 77d43b1f()
USER32! 77d444f5()
USER32! 77d44525()
NTDLL! 77f75da3()
USER32! 77d43ddf()
nsAppShellService::Run(nsAppShellService * const 0x00a4b238) line 484
main1(int 1, char * * 0x002e2638, nsISupports * 0x009aef88) line 1291 + 32 bytes
main(int 1, char * * 0x002e2638) line 1678 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e814c7()

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the problem is that aContext->GetParent() is null in
nsRuleNode::ComputeBorderData and that it's not null-checked in the           

  border->SetBorderColor(side, parentContext->GetStyleColor()->mColor);

line.

Now that parent is null because 
nsComputedDOMStyle::GetStyleData is broken like that.... it should be doing
something similar to what nsInspectorCSSUtils::GetStyleContextForContent.

In fact, would it be truly evil to use nsInspectorCSSUtils inside
nsComputedDOMStyle?

Comment 4

[Christopher Aillon (sabbatical, not receiving bugmail)]

I'd like to rename nsIInspectorCSSUtils anyway, because I think that things
there can be potentially used by non-inspector consumers.

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

And the fact that we have an interface in layout specific to inspector, if only
by name, is not really cool, either.

Comment 6

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

This is a bug in nsRuleNode -- 'border-color: inherit' on the root needs to set
the foreground value.  We might want to fix nsStyleBorder's constructor to
initialize to the correct defaults as well...

Comment 7

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

Attachment: testcase for 'border-color: inherit' on root element

A fix for this bug should probably also fix this testcase, and should
definitely not make it crash.

Comment 8

[Stephen Oberholtzer]

Personally, I think anyone who specifies 'inherit' for a property on the root
node should be shot.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Personally, I think anyone who doesn't realize that a rule can match multiple
nodes _including_ the root node.... well, you know.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Of course in that testcase the style context of the <html> _does_ have a
parentContext (probably the scrollframe or some silly thing like that).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yep.

      // resolve a new style for the root frame
      rootPseudoStyle = styleSet->ResolvePseudoStyleFor(nsnull,
                                                        rootPseudo,
                                                        scrollPseudoStyle);

This is in nsCSSFrameConstructor::ConstructRootFrame

Comment 12

[Boris Zbarsky [:bzbarsky]]

Er, that's not the style for the root node.. silly me.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Rather evil patch

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143458 [details] [diff] [review]
Rather evil patch

So the nsInspectorCSSUtils changes are just to avoid duplication of code.  The
nsComputedDOMStyle changes are to make sure we get the right style.  The
nsCSSFrameConstructor changes are to make the display of dbaron's testcase
right, and the nsRuleNode changes fix the crash.

Comment 15

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

Comment on attachment 143458 [details] [diff] [review]
Rather evil patch

>Index: content/html/style/src/nsInspectorCSSUtils.cpp
> nsInspectorCSSUtils::GetStyleContextForContent(nsIContent* aContent,
>-    // No frame has been created, so resolve the style ourselves
>+    // No frame has been created or we have a pseudo, so resolve the
>+    // style ourselves
>     nsRefPtr<nsStyleContext> parentContext;
>     nsCOMPtr<nsIContent> parent = aContent->GetParent();

should be = aPseudo ? aContent : aContent->GetParent();

>     nsRefPtr<nsStyleContext> sContext =
>-        GetStyleContextForContent(aContent, presShell);
>+        GetStyleContextForContent(aContent, nsnull, presShell);

Given that this was the only user, the version in nsComputedDOMStyle would have
been sufficient for inspector (but not for computed style!).

>Index: layout/html/style/src/nsCSSFrameConstructor.cpp
>   styleContext = aPresShell->StyleSet()->ResolveStyleFor(aDocElement,
>-                                                         aParentStyleContext);
>+                                                         nsnull);

To match this, do you need to fix implementations of
nsIFrame::GetParentStyleContextFrame ?

Other than that, looks good, although a diff -w would have been nice.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> should be = aPseudo ? aContent : aContent->GetParent();

Doh.  Indeed.

> To match this, do you need to fix implementations of
> nsIFrame::GetParentStyleContextFrame ?

Yes, I do.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments, diff -w (FOR REVIEW ONLY)

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Same, but without -w for checkin

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 143666 [details] [diff] [review]
Updated to comments, diff -w (FOR REVIEW ONLY)

>-    // No frame has been created, so resolve the style ourselves
>+    // No frame has been created or we have a pseudo, so resolve the
>+    // style ourselves
>     nsRefPtr<nsStyleContext> parentContext;
>-    nsCOMPtr<nsIContent> parent = aContent->GetParent();
>+    nsCOMPtr<nsIContent> parent = aPseudo ? aContent : aContent->GetParent();

I don't think this needs to be a strong reference, does it?

r=caillon

Comment 20

[Boris Zbarsky [:bzbarsky]]

You're right, it does not.  I'll make that change before checking in.

Targeting at 1.8a, but if someone feels this should go into 1.7b, please speak
up (and request approval)....

Comment 21

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 143666 [details] [diff] [review]
Updated to comments, diff -w (FOR REVIEW ONLY)

Yeah, I think this crash fix would be good to get for beta if we can.  From an
extension standpoint, this will make inspector not suck as much for 1.7 final. 
Otherwise, users who want this crash fix in a "stable" release will have to
wait for 1.8.  Anyway, this shouldn't be hit too much by websites apart from
the rule node change (which should be safe IMO), as its hit mostly from
computed style code so it would be good to get what exposure we can in a beta. 
If problems arise from this, I'll be happy to back it out.

Comment 22

[Asa Dotzler [:asa]]

Comment on attachment 143666 [details] [diff] [review]
Updated to comments, diff -w (FOR REVIEW ONLY)

a=asa (on behalf of drivers) for checkin to 1.7

Comment 23

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.7.

Comment 24

[[e-mail]]

Might not be relevant, but this is new for me:
###!!! ASSERTION: Cannot find NonTransparentBackground in a null context:
'context', file nsCSSRendering.cpp, line 2497

Comment 25

[steve.swanson]

Since pulling yesterday shortly after this landed, I'm been getting ASSERTs 
whenever I load a document.  On the Mozilla start page, it happens about 18 
times.  On others, I just give up.

---------------------------
nsDebug::Assertion
---------------------------
ASSERTION: Cannot find NonTransparentBackground in a null context: 'context', 
file c:/Mozilla/mozilla/layout/html/style/src/nsCSSRendering.cpp, line 2497

Windows XP, VC7 2003

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix assert

This should fix the assert (which was being triggered by the root's context
being passed in with aStartAtParent true).  aStartAtParent is a quirks hack in
any case, so not applying that quirk to the root seems reasonable to me.

All that said, this whole method looks totally bogus.  We should be walking up
the parent tree, not the style context tree, in my opinion... as things
currently stand, "special" beveled borders on positioned elements depend on the
background color of the parent, not of the containing block.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145137 [details] [diff] [review]
Fix assert

Could this please be approved for 1.7?	Fixes an assert that got triggered as a
result of the first patch in this bug...

Comment 28

[chris hofmann]

Comment on attachment 145137 [details] [diff] [review]
Fix assert

a=chofmann for 1.7

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 145137 [details] [diff] [review]
Fix assert

Checked in for 1.7.

Comment 30

[steve.swanson]

Thanks.  Works nicely.

Comment 31

[Henrik Gemal]

when compiling on gcc on winxp:

d:/mozilla/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp:4566: undefin
ed reference to `NS_NewObjectFrame(nsIPresShell*, nsIFrame**)'
../../dist/lib/libgkhtmlstyle_s.a(nsCSSFrameConstructor.o)(.text+0xaf10):d:/mozi
lla/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp:4574: undefined refe
rence to `NS_NewObjectFrame(nsIPresShell*, nsIFrame**)'
../../dist/lib/libgkhtmlstyle_s.a(nsCSSFrameConstructor.o)(.text+0xaf69):d:/mozi
lla/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp:4581: undefined refe
rence to `NS_NewObjectFrame(nsIPresShell*, nsIFrame**)'

Comment 32

[Boris Zbarsky [:bzbarsky]]

What does that have to do with this bug, pray tell?

Comment 33

[Henrik Gemal]

this bug had a checkin to the file nsCSSFrameConstructor.cpp and now I cant
compile...:(

Comment 34

[Boris Zbarsky [:bzbarsky]]

Yes, but your problem is in code pretty totally unrelated to the code touched by
this bug.