196603 - devedge.netscape.com - menus and "customize" do not work in recent nightlies

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Marek Stępień [:marcoos, inactive]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4a) Gecko/20030308
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4a) Gecko/20030308

The redesigned devedge.netscape.com does not work properly in recent nightlies.
It uses Eric Meyer's CSS menu, but modified with some JavaScript to show the
pull down menus and to display a <div> with the option to change stylesheet.
This worked in 20030303 nightly build of Mozilla, and does not work
in the following builds:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4a) Gecko/20030308
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3b) Gecko/20030307 Phoenix/0.5 

Reproducible: Always

Steps to Reproduce:
1. Go to http://devedge.netscape.com/
2. Hover the "Customize" button [1] or the menu ("View Source | Archive | Tech
Centrall") [2]


Actual Results:  
Nothing happens.

Expected Results:  
Show the <div> with option to change the stylesheet [1]
or display the contents of the menu [2].


I was also notified by MozillaZine readers (
http://www.mozillazine.org/forums/viewtopic.php?t=7089 ), that this problem is
reproducible also in:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3b) Gecko/20030308 Phoenix/0.5

but the page works fine in
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4a) Gecko/20030306

so this has become broken probably on March 7th or March 8th.

Comment 1

[Chris Casciano]

WFM branch build:
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3) Gecko/20030308

Comment 2

[Marek Stępień [:marcoos, inactive]]

Yes, I forgot to mention here, that this problem exists only in 1.4a trunk
builds, not in the 1.3 branch.

Comment 3

[Andrew Schultz]

confirmed with linux turnk 20030308
regression bewteen linux trunk 2003030605 and 2003030705

==> Layout

Comment 4

[Andrew Schultz]

Attachment: testcase

mousing over "International" works the first time, mousing out hides it, but
after that it stops working.

Comment 5

[Andrew Schultz]

regression from bug 171830?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Hmm.. ok, I see the problem on the devedge site.  I have to try very hard to get
the testcase to reproduce it (have to move over to the right, not down; then
sometimes the mouseout fires without me actually mousing out of the <p> and the
bug appears).

ccing jkeiser in case he knows why the mouseout fires when it should not.....

In any case, I'm not likely to be able to seriously debug this for at least a
few more days....

Comment 7

[Andrew Schultz]

backing out bug 171830 fixed the URL and testcase

Comment 8

[David Baron :dbaron:]

The patch for bug 171830 *should* have changed what happens here from a reflow
to a repaint -- but could it somehow have caused a reframe instead?

Comment 9

[Boris Zbarsky [:bzbarsky]]

OK, so I think I sorta know what's going on...  Here's what I see when I stick
some printfs in the code (the ParsePropertyValue is in
nsDOMCSSAttrDeclaration.cpp, the other is in the frame constructor; MaxHint is
the hint the reresolve returned).

ParsePropertyValue 'padding-bottom' to '0' Old Value '0pt'
MaxHint: 14
ParsePropertyValue 'left' to '6px' Old Value '6px'
MaxHint: 14
ParsePropertyValue 'top' to '22px' Old Value '22px'
MaxHint: 14
ParsePropertyValue 'visibility' to 'visible' Old Value 'hidden'
MaxHint: 14

So the problem is in fact that we just don't repaint that area (going to a
different virtual terminal and coming back make the menu show up fine).

If I change nsStyleContext::CalcStyleDifference to use GetStyleData throughout
instead of using PeekStyleData, I get the right change hints and the right behavior.

What I suspect is happening is this.

1)  We start out with a style context (Context1).
2)  We change some inline style, creating Context2; diff context1 and context2
    and _stop_ at the point when we find what the difference is (when the
    optimizations in CalcStyleDifference kick in).  We post a reflow or
    invalidate or something for the frame.
3)  Before this is processed, we change some more inline style creating
    Context3; we diff context2 and context3.  Now the reflow/repaint/whatever
    has not happened yet, so we only have the style structs on context2 that
    were created in step #2.  None of those structs changed, so we move on down
    the list and see no change at all.

E.g. if step #2 triggered a reflow and step #3 triggered a reflow or repaint
further down the list, the second reflow or repaint would not get processed
properly...

Another possibility is that the hint from step #2 _is_ being processed fully,
but just not requesting all the structs that it probably should....

Comment 10

[Boris Zbarsky [:bzbarsky]]

Needless to say, a minimal testcase that demonstrates the problem consistently
(as the page does) would help immensely in debugging this.... ;)  I'll try to
construct one tomorrow, but if someone has time before that and is willing to do
it....

Comment 11

[Andrew Schultz]

the testcase works consistently for me.  The one thing to watch out for is that
if you mouseover the hidden element (Français), it will become visible.  Also
the mouseover area is 100% width, so mouseover left-right is not real effective
unless your mouse actually leaves the Mozilla window.  Coming in from the top
and then leaving through the top triggers the bug every time for me.

(I'm a bit confused by "have to move over to the right, not down")

Comment 12

[Andrew Schultz]

Attachment: testcase that's more clear

with this testcase, it should be easier to see what is and is not happening.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Thanks!  Yeah, I see the problem with the first testcase if I go up.

Will try to debug some more later this week....

Comment 14

[Pascal Chevrel:pascalc]

Seeing the problem with build 2003031208 on WinXP too, changing OS to ALL

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: smaller testcase

OK, this one is probably about minimal...

This testcase, and others in this bug, fail even if I change from PeekStyleData
to GetStyleData, though that change fixes the devedge site.

Notes:

1)  I can reproduce the bug with visibility or -moz-opacity, but not with
color;
    I'm betting that's because the color struct is after the padding struct in
    the order we compute changes in.
2)  When I add a printf to the point right after I get the "otherVis" struct in

    nsStyleContext::CalcStyleDifference as follows:

      fprintf(stderr, "this: %p, other: %p, vis: %p, othervis: %p, diff: %d\n",

		       this, aOther, vis, otherVis,
		       vis->CalcDifference(*otherVis));

    I get the following when I mouseover the text:

this: 0x876c040, other: 0x876c5f4, vis: 0x862cfa8, othervis: 0x862cfa8, diff: 0

this: 0x876c768, other: 0x876c140, vis: 0x876c200, othervis: 0x876c7c8, diff: 0


and the following when I mouseout:

this: 0x876c140, other: 0x876c794, vis: 0x876c7c8, othervis: 0x876c200, diff:
94

If I take out the line that sets the padding, I instead get:

this: 0x87658e0, other: 0x8765d44, vis: 0x8765860, othervis: 0x8765ca4, diff:
94 (mouseover)
this: 0x8765d44, other: 0x8765834, vis: 0x8765ca4, othervis: 0x8765860, diff:
94 (mouseout)

It may be that caching the style structs themselves somewhere in the ruletree,
so that even though the content gets a new style rule and hence a new
rulenode/stylecontext it's ending up with the same struct as it used to
have.... Not sure why that's breaking things, though....

Comment 16

[Chris Casciano]

Setting platform to All (Mac/OS X)

Comment 17

[Pham]

I'm seeing this too. What I can do is hover over the menu(download etc) and
press ctrl+ -/= and the menu drops down, but when I go over them, they
dissapear. This problem also dissapears if I disable javascript for navigator
for the moment.

Comment 18

[Boris Zbarsky [:bzbarsky]]

You can't set that flag if you're not a driver.  Or rather you can, but you'll
be ignored.

David, do you have any idea what's up here?  I may be able to debug this on
Tuesday a bit, but then I'm gone till early April...

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK, here's what's going on:

1)  We change padding on the parent node.   This triggers a style reresolve down
    the content/frame tree from that point.  All the kids get new style
    contexts, but none of those style contexts get any structs, because of the  
    "it's in the same position in the rule tree and has the same rulenode"
    optimization.
2)  Since the change hint comes out to NS_STYLE_HINT_NONE, there is no actual
    reflow or anything, so those structs remain unfilled (before bug 171830 the
    hint here would have been REFLOW).
3)  Another change comes along (for visibility) targeted at one of the kids and
    the structs are either null (since they don't exist) or get computed based
    on the nsCSSDeclaration. But the nsCSSDeclaration has _changed_ (since we
    don't clone the decl).  So our change hint here is always
    NS_STYLE_HINT_NONE.

Removing the optimization in item #1 fixes this bug (since the structs are now
filled).  Just cloning the decl fixes some testcases, but not all.  Just using
GetStyleData instead of PeekStyleData fixes some testcases, but not all. 
Cloning the decl _and_ using GetStyleData does fix all the testcases (since it
forces the creation of structs with the right style data in them).

Ho-hum.  So thoughts?  Note that we've always ended up in this situation where
style structs are blown away from child contexts by a parent's inline style
changing and things are broken thereafter; it just didn't affect inline style
changes, since those didn't depend on the resolved change hint (but _did_
affect, eg, :hover, alternate sheet switches, etc).

Comment 20

[Boris Zbarsky [:bzbarsky]]

So to sum up the situation, there are three possible approaches to the fix that
I can think of:

1)  Go back to doing a reflow even in cases when it's not really needed
2)  Make sure the new style contexts created for the kids in Step 1 of comment
    19 have all the needed style structs.  One way would be to change the
    optimization to always GetStyleData if PeekStyleData returned something but
    to only call CalcDifference if the rulenodes are different.
3)  Make sure that we don't touch the old style data in any way (Clone() the
    nsCSSDeclaration in inline style changes) and replace PeekStyleData with
    GetStyleData.

I'm not actually sure which of these is most performant.....  My personal
preference lies with #2, sort of, but real style rule immutability would sorta
point to #3....  Of course then cloning an nsCSSDeclaration needs to be cheap.

Comment 21

[David Baron :dbaron:]

I think I prefer #2 as well.  (Doesn't that just involve removing the
"break-out" optimization in CalcStyleDifference by moving the NS_IsHintSubset
tests in with the |struct != otherStruct| test?)  How does that disagree with
style rule immutability?

Comment 22

[Boris Zbarsky [:bzbarsky]]

Actually, it means combining the mRuleNode == aOther.mRuleNode test with the
struct != otherstruct test.

We could move the hint subset stuff there too, but if that test fails then we'll
be doing reflow or whatever and reconstructing the structs anyway (or so the
theory goes).  It's safer to combine all three tests, of course....

The immutability issue is that in this case we _are_ mutating the old
nsIStyleRule.  But I guess it does not matter since we also stop pointing to it
once the re-resolve completes...

OK, so let's go with #2.  David, will you have time to do this in the next day
or two?  If not, I can implement it on Tuesday.

Comment 23

[David Baron :dbaron:]

I'll attach a patch to do #2 shortly (once my build finishes and I can test it).

Comment 24

[David Baron :dbaron:]

Attachment: patch

OK, this is fun.  If I have porting trouble with this, I'll add a bogus 6th
parameter to the inline function for type deduction and get rid of the explicit
typing.

Comment 25

[David Baron :dbaron:]

Actually, hold on, there's a slightly better way to do this...

Comment 26

[David Baron :dbaron:]

Actually, never mind.

Comment 27

[David Baron :dbaron:]

But when I check in I should remember to add the accessor to nsStyleSVG, since
for some reason it doesn't have one.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 117333 [details] [diff] [review]
patch

nice! But I also wonder about the portability of the explicit template
parameter.

Comment 29

[David Baron :dbaron:]

We use the trick of adding an extra parameter to template functions all over in
the string code, so it clearly works on our set of compilers.

Comment 30

[David Baron :dbaron:]

Attachment: potential bustage fix

Here's what I'd do if there are problems with what I checked in (in case some
obscure port breaks in the middle of the night after 5 hours thinking about
it).

Comment 31

[David Baron :dbaron:]

I used the potential bustage fix since IRIX and OS/2 VACPP didn't like the other
way.

Comment 32

[David Baron :dbaron:]

And from the codesize diffs it looks like VC++ (Windows) compiled it but did the
wrong thing.

Comment 33

[David Baron :dbaron:]

Well, fix checked in to trunk (although given the state of optimization on
current C++ compilers, judging from codesize logs, I'm tempted to just convert
this to macros).

Comment 34

[David Baron :dbaron:]

Attachment: macro-ization

Is this better or worse?  It probably messes up debuggers more, but it should
reduce the codesize penalty...

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is ugly ... are you going to just check this in to see what it does to code
size on various platforms?

I don't think you need to whack the names of 'this' and 'other', since they're
scoped to the enclosing block. That would reduce the ugliness a tiny bit.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The code looks good though, r+sr etc

Comment 37

[David Baron :dbaron:]

I vaguely remember some compiler not liking variables of the same name across
scoped blocks.  It's a pretty vague memory, though.

I'm pretty sure it will improve codesize.  It looks like Visual C++ isn't inlining.

Comment 38

[Brendan Eich [:brendan]]

Comment on attachment 117359 [details] [diff] [review]
macro-ization

It's better style to use PR_BEGIN_MACRO and PR_END_MACRO instead of braces --
then you can use ; after each call, even in an unbraced then statement between
if (condition) and else.

/be

Comment 39

[David Baron :dbaron:]

bryner said Windows builds were having serious problems even with the second
patch (I presume due to some compiler stupidity), so I checked in the macroization.