158230 - The expand/collapse icon disappears

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Prashant Desale]

The Parralel bugscape bug is 17944

BUILDS: Tested on [2002-07-17-08-1.0/-windows] & [2002-07-17-07-1.0/-Linux]

STEPS TO REPRODUCE:
1] Please Visit URL http://bubblegum.mcom.com/desale/dig-bugs/one/DigTree.htm
2] Click on the "-" icon to collapse the children in the tree
3] Click on the "+" icon to expand the children

EXPECTED RESULTS:
Tree should be expanded & expand/collapse icon should not disappear.

ACTUAL RESULTS
The expand/collapse icon disappears

So far I've seen this on windows & Linux hence I'm marking OS as ALL.

Comment 1

[Prashant Desale]

adding sandip to cc

Comment 2

[lchiang]

Actually, the URL should be
http://www.mozilla.org/quality/ngdriver/suites/desale/one/DigTree.htm and not
the one referenced in the original steps.

Comment 3

[Prashant Desale]

This is regression from 6.2

Sorry I apologize for wrong URL mentioned in Steps to reproduce.
As Lisa said, the test URL should be
http://www.mozilla.org/quality/ngdriver/suites/desale/one/DigTree.htm

Comment 4

[Prashant Desale]

This is regression from 6.2, hence adding regression keyword.

Comment 5

[Jaime Rodriguez, Jr.]

--> cathleen, as dbaron is on vacation.  can we find another owner for this one,
in david's absence?

Comment 6

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

Does anyone have a better idea of when the regression occurred?  (I'd guess it
would be related either to changes related to the undisplayed map or to changes
related to image placeholders.)

Comment 7

[kinmoz]

So looking at the test case source, I noticed that several elements have the
same id attribute ... I thought all elements with an id attribute had to have
unique ids?

Comment 8

[kinmoz]

FYI, the test case works fine if you remove the <a> link around the +/- buttons.

Comment 9

[Paul Wyskoczka]

The regression is from the checkin for bugzilla bug 146831 - 90% cpu useage on
netscape.com

Comment 10

[kinmoz]

FYI, I backed out dbaron's changes for bug 146831 in my local tree and I still
see this bug.

Comment 11

[lchiang]

prashant - can you go back one month on a build (non-trunk, if branch/trunk had
already happened) and start to isolate when this bug first occurred?

Thanks

Comment 12

[Prashant Desale]

I DO NOT think this one is caused because if checkin for bugzilla bug 146831.
I Performed Tests on all the builds around when checkin for bugzilla bug# 146831
happened. Also I tested 7.0PR1. Tests were performed on win2k & I found
following results.

BUILD                    RESULTS         
7.0PR1                   Fails.  [Will try to findout when it started happening.] 
2002-06-19-08-1.0/       Fails.
2002-06-20-08-1.0/       Fails.
2002-06-21-08-1.0/       Fails. 

This proves that this particular bug DID NOT occured due to checkin for bugzilla
bug 146831.

Comment 13

[kinmoz]

Attachment: Test Case

This is the test case waterson and I have been debugging with.

Here's some of my notes:

Notes for waterson ...

Just before the image disappears, when I click on the "+" image a
ContentStatesChanged() call gets triggered. I believe waterson figured out it
was the rule in html.css that draws the border around an image inside a link.

When aPresContext->ResolveStyleContextFor(content, parentContext, &newContext)
for the image currenlty on screen, it's returning a newContext that has a
display of "none", which causes CalcDifference() to return a min change of
NS_STYLE_HINT_FRAMECHANGE:



nsStyleDisplay::CalcDifference()
nsStyleContext::CalcStyleDifference()
CaptureChange()
FrameManager::ReResolveStyleContext()
FrameManager::ReResolveStyleContext()
FrameManager::ComputeStyleChangeFor()
nsCSSFrameConstructor::ContentStatesChanged()
StyleSetImpl::ContentStatesChanged()
PresShell::ContentStatesChanged()
nsDocument::ContentStatesChanged()
nsEventStateManager::SetContentState()
nsGenericHTMLElement::HandleDOMEventForAnchors()
nsHTMLAnchorElement::HandleDOMEvent()
nsGenericElement::HandleDOMEvent()
nsHTMLImageElement::HandleDOMEvent()
PresShell::HandleEventInternal()
PresShell::HandleEvent()
nsViewManager::HandleEvent()
nsView::HandleEvent()
nsViewManager::DispatchEvent()
HandleEvent()
nsWindow::DispatchEvent()
nsWindow::DispatchWindowEvent()
nsWindow::DispatchMouseEvent()
ChildWindow::DispatchMouseEvent()
nsWindow::ProcessMessage()
nsWindow::WindowProc()
USER32! 77e11b60()
USER32! 77e11cca()
USER32! 77e183f1()
main()
mainCRTStartup()


This causes the image frame to get destroyed when it tries to recreate the
frame, but since the new context has a display none, a new one is never
created:



nsImageFrame::~nsImageFrame()
nsImageFrame::`scalar deleting destructor'()
nsFrame::Destroy()
nsImageFrame::Destroy()
nsFrameList::DestroyFrame()
nsInlineFrame::RemoveFrame()
FrameManager::RemoveFrame()
nsCSSFrameConstructor::ContentRemoved()
nsCSSFrameConstructor::RecreateFramesForContent()
nsCSSFrameConstructor::ProcessRestyledFrames()
nsCSSFrameConstructor::ContentStatesChanged()
StyleSetImpl::ContentStatesChanged()
PresShell::ContentStatesChanged()
nsDocument::ContentStatesChanged()
nsEventStateManager::SetContentState()
nsGenericHTMLElement::HandleDOMEventForAnchors()
nsHTMLAnchorElement::HandleDOMEvent()
nsGenericElement::HandleDOMEvent()
nsHTMLImageElement::HandleDOMEvent()
PresShell::HandleEventInternal()
PresShell::HandleEvent()
nsViewManager::HandleEvent()
nsView::HandleEvent()
nsViewManager::DispatchEvent()
HandleEvent()
nsWindow::DispatchEvent()
nsWindow::DispatchWindowEvent()
nsWindow::DispatchMouseEvent()
ChildWindow::DispatchMouseEvent()
nsWindow::ProcessMessage()
nsWindow::WindowProc()
USER32! 77e11b60()
USER32! 77e11cca()
USER32! 77e183f1()
main()
mainCRTStartup()

Comment 14

[Prashant Desale]

I tried this one on multiple builds to trackdown when this first occured & I
could get to approximate dates between when this one possible first time occured.
This one occured sometime between  03/07/2002 & 03/26/2002

I tested following builds & here are results:

BUILDS FROM SWEETLOU             RESULTS
6.2RTM                           Works Fine.
7.0PR1                           Fails. 
2002-06-19-08-1.0/               Fails.
2002-06-20-08-1.0/               Fails.
2002-06-21-08-1.0/               Fails. 
2002-04-13-1.0.0                 Fails.
2002-04-01-06-trunk              Fails.

On sweetlou trunk was available only till 2002-04-01-06-trunk.

There were some old trunks available on jrgm's machine & I tested there and
found following Results.

BUILDS ON LOCAL MACHINE             RESULTS
2002-02-12-trunk                    Works Fine.
2002-03-07-trunk                    Works Fine.
2002-03-26-trunk                    Fails.

Above results show that this one occured sometime between  03/07/2002 & 03/26/2002

Comment 15

[Chris Waterson]

It looks like the reason we're ending up with a NS_STYLE_DISPLAY_NONE is because
of the inline style rule that exists on the image element that was originally
"style='display: none'".

As far as I can tell, the problem occurs in nsRuleNode::Transition because the
rule node for this element 1) already has children and 2) we end up finding a
"next" (sorry for the shallow description; I'm not exactly sure how this stuff
works). This means we will not create a new rule node, and instead use the
original rule node, which has a dipslay of NS_STYLE_DISPLAY_NONE.

As kin notes, above, the effect is that the ESM processing of the onclick (whose
only function ought to be to make the link element :active) winds up destroying
the image frame because of the overzealous FRAMECHANGE indication.

I'm going to now try to understand a bit more about how inline style rules are
supposed to work.

Comment 16

[David Hyatt]

This could have to do with the branch not getting cleared.  My guess would be 
that my code that maps to a list of style rules might be buggy.  That was one 
of the last things I added to the rule tree to fix problems similar to this 
one.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: same testcase, but add |style=""| to first image (and fix image urls)

This testcase does _not_ reproduce the problem.  So the issue has to do with
the need to create that style rule for the first image.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Testing my old trunk buids:

2002-03-11-08-trunk                    Works Fine.
2002-03-13-08-trunk                    Fails.

Comment 19

[Chris Waterson]

Okay, I'm going to take a stab and say that the problem could be with
nsCSSFrameConstructor::AttributeChanged. At around line 10617
(MOZILLA_1_0_BRANCH, rev. 1.727.2.13), we only blow away cached rule node
information if the element has a "style" attribute. We probably need to check
here for _any_ inline style.

Comment 20

[Chris Waterson]

Oh, no. Nevermind. It looks like nsDOMCSSDeclaration::GetCSSDeclaration should
take care of forcing this to be allocated.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: One possible patch

The basically changes ReconstructFramesForContent to do the same style
invalidation that AttributeChanged does (removing dbarons's XXX comment,
basically).  This patch fixes this bug (and the debug output about misparented
style contexts goes away), but I don't quite understand why yet...  Other data:


1)  If aStyleContext and aInlineStyleRule are both non-null (which they are in
    this case), nsIStyleSet::ClearStyleData() does about the same thing as
    nsIStyleContext::ClearCachedDataForRule() with one key difference -- the
    nsIStyleSet function looks at the style set's mRuleMappings, which is
    probably the key here.
2)  If aStyleContext is not null but aInlineStyleRule is null, the two
functions
    are identical.
3)  If aStyleContext is null, the ClearStyleData call does the same as
    ClearCachedDataInRuleTree, except it also clears any style contexts
affected
    by the rule in question.  This last bit is unnecessary in this case, most
    likely.
4)  This point in the code is the _only_ place where ClearCachedDataForRule()
    and ClearCachedDataInRuleTree() get called.

I suspect the right thing to do is to just use nsIStyleSet::ClearStyleData
universally, remove the other two functions, and add an arg to ClearStyleData
that is true for inline style that will affect the way the "no style context"
case is handled (per dbaron's XXX comment in
nsCSSFrameConstructor::AttributeChanged, above the ClearStyleData call), if
that case needs to be handled differently for inline style.

Thoughts?  hyatt?  You know this stuff, right?	;)

Comment 22

[Chris Waterson]

bz, that sounds sane to me, from the little that I've learned about this stuff
today. Any chance you could backtrace throught the bugscape bug to see if any of
the other DIG problems go away?

<http://bugscape.netscape.com/show_bug.cgi?id=17841>

Comment 23

[Boris Zbarsky [:bzbarsky]]

Tried that.  I cannot reproduce any of the DIG problems other than this one in a
trunk build.  I don't have an up-to-date branch build, but I'll kick one off...

Comment 24

[David Hyatt]

This patch looks good to me.  sr=hyatt

Comment 25

[David Hyatt]

Comment on attachment 92346 [details] [diff] [review]
One possible patch

sr=hyatt

Comment 26

[Boris Zbarsky [:bzbarsky]]

Ok.  I just finished my branch build (Mozilla, not commercial).  Again, this
problem is the only one of the DIG problems that I can reproduce.

Comment 27

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

Comment on attachment 92346 [details] [diff] [review]
One possible patch

r=dbaron, but perhaps you should copy the XXX comment from AttributeChanged as
well (about the null aStyleContext)?  (I'm assuming the normal case is that
|aStyleContext| is non-null.)

Comment 28

[Boris Zbarsky [:bzbarsky]]

Yeah, I'll copy the comment before checking in.

Comment 29

[Asa Dotzler [:asa]]

Comment on attachment 92346 [details] [diff] [review]
One possible patch

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

Comment 30

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk.

Waiting on branch approval from drivers/adt, now.

Do we want to also remove the now-not-used functions on trunk?

Comment 31

[Paul Wyskoczka]

Adding adt1.0.1+ on behalf on the adt.  Please get drivers approval before
checking into the branch. 

Comment 32

[Judson Valeski]

please checkin to the 1.0.1 branch. once there, remove the "mozilla1.0.1+"
keyword and add the "fixed1.0.1" keyword.

Comment 33

[Boris Zbarsky [:bzbarsky]]

checked in on the branch.

Comment 34

[Jaime Rodriguez, Jr.]

desale: prashant, can you pls verify this as fixed on the branch tomorrow, then
add the keyword "fixed1.0.1"? thanks!

Comment 35

[Prashant Desale]

Awsome. It works on 2002-07-24-08-1.0/ Marking verified1.0.1.

Comment 36

[Randell Jesup [:jesup] (needinfo me)]

Adding fixed1.0.1 as well
Marking fixed.  BZ, please add a new bug against trunk to remove the now-unused
functions.

Comment 37

[Boris Zbarsky [:bzbarsky]]

*** Bug 159197 has been marked as a duplicate of this bug. ***

Comment 38

[Prashant Desale]

Its already verified1.0.1, so removing fixed1.0.1 keyword.
Marking it verified too.

Comment 39

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

*** Bug 141304 has been marked as a duplicate of this bug. ***

Comment 40

[Boris Zbarsky [:bzbarsky]]

Filed bug 169989 per comment 36.