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

RESOLVED FIXED in mozilla1.8alpha1

Status

Other Applications
DOM Inspector
P2
critical
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: Stephen Oberholtzer, Assigned: bz)

Tracking

Trunk
mozilla1.8alpha1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

14 years ago
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 :)
(Reporter)

Comment 1

14 years ago
Created attachment 143401 [details]
Example page that demonstrates the bug

Comment 2

14 years ago
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()
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
I'd like to rename nsIInspectorCSSUtils anyway, because I think that things
there can be potentially used by non-inspector consumers.
And the fact that we have an interface in layout specific to inspector, if only
by name, is not really cool, either.
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...
Created attachment 143451 [details]
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.
(Reporter)

Comment 8

14 years ago
Personally, I think anyone who specifies 'inherit' for a property on the root
node should be shot.
Personally, I think anyone who doesn't realize that a rule can match multiple
nodes _including_ the root node.... well, you know.
Of course in that testcase the style context of the <html> _does_ have a
parentContext (probably the scrollframe or some silly thing like that).
Yep.

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

This is in nsCSSFrameConstructor::ConstructRootFrame
Er, that's not the style for the root node.. silly me.
Created attachment 143458 [details] [diff] [review]
Rather evil patch
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.
Attachment #143458 - Flags: superreview?(dbaron)
Attachment #143458 - Flags: review?(caillon)
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.
> should be = aPseudo ? aContent : aContent->GetParent();

Doh.  Indeed.

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

Yes, I do.
Created attachment 143666 [details] [diff] [review]
Updated to comments, diff -w (FOR REVIEW ONLY)
Attachment #143458 - Attachment is obsolete: true
Created attachment 143668 [details] [diff] [review]
Same, but without -w for checkin
Attachment #143666 - Flags: superreview?(dbaron)
Attachment #143666 - Flags: review?(caillon)
Attachment #143458 - Flags: superreview?(dbaron)
Attachment #143458 - Flags: review?(caillon)
Attachment #143666 - Flags: superreview?(dbaron) → superreview+
Assignee: dom-inspector → bzbarsky
Summary: CRASH when inspecting div with border-color inherit → [FIX]CRASH when inspecting div with border-color inherit
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
Attachment #143666 - Flags: review?(caillon) → review+
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)....
OS: Windows 2000 → All
Priority: -- → P2
Hardware: PC → All
Summary: [FIX]CRASH when inspecting div with border-color inherit → [FIXr]CRASH when inspecting div with border-color inherit
Target Milestone: --- → mozilla1.8alpha
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.
Attachment #143666 - Flags: approval1.7b?
Attachment #143666 - Flags: approval1.7b? → approval1.7?

Comment 22

14 years ago
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
Attachment #143666 - Flags: approval1.7? → approval1.7+
Checked in for 1.7.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 24

14 years ago
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

14 years ago
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

Created attachment 145137 [details] [diff] [review]
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.
Attachment #143666 - Attachment is obsolete: true
Attachment #143668 - Attachment is obsolete: true
Attachment #145137 - Flags: superreview?(dbaron)
Attachment #145137 - Flags: review?(dbaron)
Attachment #145137 - Flags: superreview?(dbaron)
Attachment #145137 - Flags: superreview+
Attachment #145137 - Flags: review?(dbaron)
Attachment #145137 - Flags: review+
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...
Attachment #145137 - Flags: approval1.7?

Comment 28

14 years ago
Comment on attachment 145137 [details] [diff] [review]
Fix assert

a=chofmann for 1.7
Attachment #145137 - Flags: approval1.7? → approval1.7+
Comment on attachment 145137 [details] [diff] [review]
Fix assert

Checked in for 1.7.
Attachment #145137 - Attachment is obsolete: true

Comment 30

14 years ago
Thanks.  Works nicely.

Comment 31

14 years ago
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**)'
What does that have to do with this bug, pray tell?

Comment 33

14 years ago
this bug had a checkin to the file nsCSSFrameConstructor.cpp and now I cant
compile...:(
Yes, but your problem is in code pretty totally unrelated to the code touched by
this bug.
Product: Core → Other Applications
QA Contact: timeless → dom-inspector
You need to log in before you can comment on or make changes to this bug.