Closed Bug 210269 Opened 21 years ago Closed 17 years ago

pageload crash [@ nsXULElement::SetAttr]

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.5final

People

(Reporter: spam, Assigned: roc)

References

(
URL
)

Details

(4 keywords)

Crash Data

Attachments

(6 files, 2 obsolete files)

stack of crash, non-debug
14.57 KB, text/plain
Details
stack of the first AddRef or Release after mDocument becomes null
33.31 KB, text/plain; charset=UTF-8
Details
how I got the stack
4.79 KB, patch
Details | Diff | Splinter Review
possible patch
2.57 KB, patch
Details | Diff | Splinter Review
band-aid fix
975 bytes, patch
dbaron
: review+
dbaron
: superreview+
asa
: approval1.5+
Details | Diff | Splinter Review
updated to trunk
33.83 KB, patch
bryner
: review+
bryner
: superreview+
Details | Diff | Splinter Review 
Day old trunk CVS, Linux

Go to http://www.sol.no/underholdning/tvguiden/
Click link upper left side:
Lag din egen TV-guide ("Make your own TV-guide")
Make a username and password - Save some channels if you get that far.
(It's cookie based.)

If you don't crash by logging in now, delete the tvg cookie, revisit and
this time click the link
"Finn igjen din TV-guide" ("Find your TV-guide again")

I never get as far as anything rendering on the page that should have followed.
1.4 is fine, so this is a regression. Able to reproduce every time.

Head of stack:

(gdb) bt
#0  0x40e62f37 in nsXULElement::SetAttr(nsINodeInfo*, nsAString const&, int) ()
   from /home/dark/MOZ/TREE1/mozilla/dist/bin/components/libgklayout.so
#1  0x40e63052 in nsXULElement::SetAttr(int, nsIAtom*, nsAString const&, int)
    () from /home/dark/MOZ/TREE1/mozilla/dist/bin/components/libgklayout.so
#2  0x40b2f16c in nsGfxScrollFrameInner::SetAttribute(nsIBox*, nsIAtom*, int,
int) () from /home/dark/MOZ/TREE1/mozilla/dist/bin/components/libgklayout.so
#3  0x40b2ebab in nsGfxScrollFrameInner::Layout(nsBoxLayoutState&) ()
   from /home/dark/MOZ/TREE1/mozilla/dist/bin/components/libgklayout.so
#4  0x40b2e1fd in nsGfxScrollFrame::DoLayout(nsBoxLayoutState&) ()
   from /home/dark/MOZ/TREE1/mozilla/dist/bin/components/libgklayout.so
#5  0x40bed668 in nsBox::Layout(nsBoxLayoutState&) ()
   from /home/dark/MOZ/TREE1/mozilla/dist/bin/components/libgklayout.so
#6  0x40bf055f in nsBoxFrame::Reflow(nsIPresContext*, nsHTMLReflowMetrics&,
nsHTMLReflowState const&, unsigned&) ()
   from /home/dark/MOZ/TREE1/mozilla/dist/bin/components/libgklayout.so
#7  0x40b2d62f in nsGfxScrollFrame::Reflow(nsIPresContext*,
nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned&) ()
   from /home/dark/MOZ/TREE1/mozilla/dist/bin/components/libgklayout.so
#8  0x40b95ed7 in nsListControlFrame::Reflow(nsIPresContext*,
nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned&) ()
   from /home/dark/MOZ/TREE1/mozilla/dist/bin/components/libgklayout.so
#9  0x40b1f0a3 in nsContainerFrame::ReflowChild(nsIFrame*, nsIPresContext*,
nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned, unsigned&) ()
   from /home/dark/MOZ/TREE1/mozilla/dist/bin/components/libgklayout.so
#10 0x40b7d593 in nsComboboxControlFrame::ReflowComboChildFrame(nsIFrame*,
nsIPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned&, int,
int) () from /home/dark/MOZ/TREE1/mozilla/dist/bin/components/libgklayout.so

    
    

    
  
turns out the logging in has nothing to do with this:
It's the tv guide in a long version that crashes.

This used to crash before too, in scroll-related code. (bug 202681 -
http://www.sol.no/tv/guiden/ )

Adding roc+moz@cs.cmu.edu to CC.
Summary: crash when logging in [nsXULElement::SetAttr] → pageload crash [nsXULElement::SetAttr]

    
    

    
  
Hmm. Does not crash with official trunk build 2003062122

    
    

    
  
Hmm.. I recently made some changes to SetAttr.... and it looks like those
changes are responsible.  Here is what I get:

(gdb) frame 7
#7  0x4134f7c1 in nsXULElement::SetAttr(nsINodeInfo*, nsAString const&, int) (
    this=0x8a532b8, aNodeInfo=0x0, aValue=@0x81870f0, aNotify=-1073775872)
    at
/home/bzbarsky/mozilla/xlib/mozilla/content/xul/content/src/nsXULElement.cpp:2463
2463            mDocument->AttributeWillChange(this, attrns, attrName);
(gdb) p mDocument
$5 = (nsIDocument *) 0x0

The relevant code is:

2461        if (mDocument && aNotify) {
2462            mDocument->BeginUpdate();
2463            mDocument->AttributeWillChange(this, attrns, attrName);
2464        }
2465

So now I am confused... none of the BeginUpdate() impls look like they should
affect mDocument.... I can add a null-check here, I guess, but I'd sorta like to
know what's going on....

    
    

    
  
Yeah, iirc implementations of nsIDocumentObserver should wait until all
EndUpdate calls are called before modifying the document, so a "wallpaper" that
just checks mDocument looks dangerous to me.

Looks like both aNodeInfo and aNotify have broken values in that call to
nsXULElement::SetAttr

    
    

    
  
Hmm... that's a good point.  That suggests that someone is clobbering the stack
or something...

Darin, does your recent parsing change fix this by any chance?

    
    

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

    
    

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

    
    

    
  
from dupes (and the fact I see this crash in Firebird 0.6.1 release on win2k),
OS->All
OS: Linux → All

    
    

    
  
TB22952115Y      Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.5b) Gecko/20030821

I found this address in Firebird Bug 216436, and promptly crashed with Mozilla.
As Talkback doesn´t show a stack, here some data from DocWatson. 
Because I can´t cut&paste DocWatson logs, until somebody tells me how, only short:

Tons of GKLAYOUT.DLL+..... Adresses, then XPCOM.DLL

GKLAYOUT.DLL+.text+0x1859b 
XPCOM.DLL!??1nsCOMPtr_base@@QAE@XZ

Found stack summary:
about 80 GKLAYOUT.DLL addresses, followed by 4 GKWIDGET.dll and 2 KERNEL32.DLL,
then 1 KERNEL(03) and 3 USER(1e)
Details available on request, watson94.wlg

    
    

    
  
Herman:
DrWatson stack traces doesn't help and general gklayout or GKWIDGET.dll  also
doesn't help.

and there is already a stack trace in comment#0...


    
    

    
  
rginda, is there a way to use venkman to find the JS caller of SetAttr here?
Blocks: 216436

    
    

    
  
this is #5 in the topcrash list
Keywords: topcrash

    
    

    
  
I suppose we could wallpaper this issue, but chances are if the stack is
corrupted it would just crash elsewhere...

I can debug this more when I get back, but not till then.

    
  
Flags: blocking1.5?

    
    

    
  
Someone should run this test with valgrind and see what comes out.

    
    

    
  
loading under valgrind asserts sometimes (RemovedAsPrimaryFrame called after
PreDestroy: 'PR_FALSE', file nsTextControlFrame.cpp, line 1323), but I can't get
it to actually crash or give any useful valgrind errors.

    
    

    
  
It's a TV schedule listing. Try register a user, add 
all channels and types of programs.

It crashes easyer if the generated page is longer.

    
    

    
  
in case you don't happen to read norwegian:
Click "Lag din egen TV-guide" (make your own TV-guide) to start making a
personalized version. On the succeeding page "Ditt navn" means "Your name".
"Ønsket passord" and "Gjenta passord" means "desired password" and "repeat
password".  From there it should be self explanatory.

    
    

    
  
is bz null check idea good?  need to move quickly if we are going to get this
one for 1.5 final

    
    

    
  
i don't think bz's nullcheck is a good idea but if it takes care of the crash
then maybe we could put it on the 1.5 branch.

Does anyone have good steps to reproduce?

    
    

    
  
I feel that the null-check is a bad idea unless we get really desperate (esp.
because, as I said, I doubt it would really help).

    
    

    
  
I can't get this to crash on Linux.

    
    

    
  
I crash about half the time loading http://www.geocaching.com/ (bug 214641).

    
    

    
  
can't get above link to crash on winxp.  using modern theme..  does't theme
selection come into play?

    
    

    
  
Chris: 2003090304/trunk/W2K crash on http://www.geocaching.com/ with Modern (->
TB23328234H) and Classic (-> TB23328317Q)

    
    

    
  
Are there cases where HTMLContentSink::BeginUpdate can cause the document to
start going away?  (meta-refresh?  setting window.location?)

    
    

    
  
I don't crash on the orginal test case bug
( http://www.sol.no/underholdning/tvguiden/ ) 
but after setting up an account and selecting some channels for my guide,
all I see is blank pages on the site..
e.g.
<html><body></body></html> 
when I was redirected to my guide or
when trying to go back to http://www.sol.no/underholdning/tvguiden/

strange...


    
    

    
  
Hmm.... it could, I suppose...

    
    

    
  
getting rid of the cookies for the site allowed me to see content there again.

    
    

    
  
At http://www.sol.no/underholdning/tvguiden/ I created an account with
everything checked (user: Mozilla, password: Mozilla) and I get the crash
reliably if I go to http://www.sol.no/underholdning/tvguiden/ , select "Fra
05:00" and "Til 23:00" and click OK.

    
    

    
  


  
I used this patch and XPCOM_DEBUG_BREAK=suspend to get the stack in the
previous attachment.

    
    

    
  
Comment on attachment 130929 [details]
stack of the first AddRef or Release after mDocument becomes null

The key points in this stack are:

>#7  0x4125299a in PresShell::SetAnonymousContentFor(nsIContent*, nsISupportsArray*) (this=0x8a24b30, aContent=0x85d8758, aAnonymousElements=0x0)
>    at /builds/trunk/mozilla/layout/html/base/src/nsPresShell.cpp:4840
>#8  0x412cde59 in nsCSSFrameConstructor::ConstructSelectFrame(nsIPresShell*, nsIPresContext*, nsFrameConstructorState&amp;, nsIContent*, nsIFrame*, nsIAtom*, nsStyleContext*, nsIFrame*&amp;, int&amp;, int, int&amp;, int, nsFrameItems&amp;) (this=0x8a2cc78,
>    aPresShell=0x8a24b30, aPresContext=0x8a5a940, aState=@0xbfff8350,
>    aContent=0x85d8758, aParentFrame=0x8a3a498, aTag=0x80fd868,
>    aStyleContext=0x8904f04, aNewFrame=@0xbfff7400,
>    aProcessChildren=@0xbfff741c, aIsAbsolutelyPositioned=0,
>    aFrameHasBeenInitialized=@0xbfff7404, aIsFixedPositioned=0,
>    aFrameItems=@0xbfff78a8)
>    at /builds/trunk/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp:4164

...

>#24 0x412e5813 in nsCSSFrameConstructor::WipeContainingBlock(nsIPresContext*, nsFrameConstructorState&amp;, nsIContent*, nsIFrame*, nsIFrame*) (this=0x8a2cc78,
>    aPresContext=0x8a5a940, aState=@0xbfff8620, aBlockContent=0x89b63a0,
>    aFrame=0x8a3a66c, aFrameList=0x8df5f04)
>    at /builds/trunk/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp:13335
>#25 0x412d8dd4 in nsCSSFrameConstructor::ContentAppended(nsIPresContext*, nsIContent*, int) (this=0x8a2cc78, aPresContext=0x8a5a940, aContainer=0x896ca00,
>    aNewIndexInContainer=14)
>    at /builds/trunk/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp:8364

...

>#29 0x4159a0bc in nsHTMLDocument::ContentAppended(nsIContent*, int) (
>    this=0x8910a50, aContainer=0x896ca00, aNewIndexInContainer=14)
>    at /builds/trunk/mozilla/content/html/document/src/nsHTMLDocument.cpp:1382
>#30 0x4158cb8f in HTMLContentSink::NotifyAppend(nsIContent*, int) (
>    this=0x8897420, aContainer=0x896ca00, aStartIndex=14)
>    at /builds/trunk/mozilla/content/html/document/src/nsHTMLContentSink.cpp:5282
>#31 0x41581aca in SinkContext::FlushTags(int) (this=0x8896c30, aNotify=1)
>    at /builds/trunk/mozilla/content/html/document/src/nsHTMLContentSink.cpp:2167
>#32 0x4158ce45 in HTMLContentSink::BeginUpdate(nsIDocument*) (this=0x8897420,
>    aDocument=0x8910a50)
>    at /builds/trunk/mozilla/content/html/document/src/nsHTMLContentSink.cpp:5374
>#33 0x413dd1ac in nsDocument::BeginUpdate() (this=0x8910a50)
>    at /builds/trunk/mozilla/content/base/src/nsDocument.cpp:1831
>#34 0x416f417a in nsXULElement::SetAttr(nsINodeInfo*, nsAString const&amp;, int) (
>    this=0x894ccd0, aNodeInfo=0x8deda20, aValue=@0xbfff8d50, aNotify=1)
>    at /builds/trunk/mozilla/content/xul/content/src/nsXULElement.cpp:2409

...

>#42 0x412b6ae8 in nsListControlFrame::Reflow(nsIPresContext*, nsHTMLReflowMetrics&amp;, nsHTMLReflowState const&amp;, unsigned&amp;) (this=0x8920490,
>    aPresContext=0x8a5a940, aDesiredSize=@0xbfff9660,
>    aReflowState=@0xbfff94f0, aStatus=@0xbfff9e60)
>    at /builds/trunk/mozilla/layout/html/forms/src/nsListControlFrame.cpp:1322


Yet another {ib} bug.

        Attachment #126227 -
        Attachment description: backtrace, non-debug → stack of crash, non-debug

    
    

    
  

      
      
      
      

        Attached patch
          
          
        possible patch
        — 
          — Splinter Review
      

    


  
This fixes the crash for me (by breaking the chain in a part of the stack that
I didn't quote).  It might be a bit of a performance improvement as well for
some things.

I don't see anything obviously broken about scrollbars after about a minute of
using Classic (with GTK native theme support) and a minute of using Modern.

    
    

    
  
Taking.

I think this fix is the right thing to do -- messing with content during reflow
is a bad idea, and I've always disliked the way the scrollbar code uses
attributes instead of member variables.  I'm somewhat surprised that it seems to
work, but if it works, I wouldn't be surprised if this isn't the only crash the
patch fixes.
Assignee: general → dbaron
Component: Browser-General → Layout: Misc Code
Priority: -- → P1
QA Contact: general → ian
Target Milestone: --- → mozilla1.5final

    
    

    
  
[16:00:09] <bryner> dbaron: i'm worried that this might break mac native scrollbars
[16:00:20] <bryner> they use nsIDocumentObserver (indirectly) to find out about
those attribute changes


bryner's probably right, but I'll test just in case...

    
    

    
  
It does fix the crash, but with the patch in attachment 130932 [details] [diff] [review], test this:

-load this bugreport
-Edit->Text Zoom->200%
-Scroll this bugpage to the absolute bottom
-Edit->Text Zoom ->100%

Here, with the patch, the page (or an area) goes black - the GTK null-color?
It refreshes first when i scroll. (Actually..ehehe..i wheel-scrolled..)

I removed the patch again and rebuilt - the black thing does not happen then.

    
    

    
  
another way to repro the blackness from comment 37 is simply to make window
heigth around half the height of monitor, scroll a page to the bottom, and then
resize the browser window by dragging bottom of it downwards. Black appears, and
the page doesn't "snap into plage" before i scroll in it.

    
    

    
  
Sounds like the code in nsSliderFrame::AttributeChanged which makes sure the
scroll position stays valid is failing to update the scroll position, which is
strange since it calls SetAttr itself with aReflow=PR_TRUE.

    
    

    
  
Yeah, it breaks scrollbars on Mac rather badly.

    
    

    
  
> Are there cases where HTMLContentSink::BeginUpdate can cause the document to
> start going away?  (meta-refresh?  setting window.location?)

I should have commented on this earlier... I think dbaron already knows this but
it should be here in the bug.

I think what's happening on this site (from investigating previous bugs here) is
that the BeginUpdate adds new content to the document and builds frames for it.
It just so happens that the new content triggers an IB situation which calls for
reframing of the parent block --- which happens to include the frame(s) we're
currently reflowing. Havoc ensues.

This crash happens a lot more often since I converted to Gfx scrollbars for
listboxes/comboboxes, because now we often have SetAttr-happy-scrollbars living
inside inline frames. This could happen before, but was rare.

I don't think this can easily be fixed just by turning off SetAttr document
notifications, because we rely on those notifications happening in some reflow
situations (e.g., when the scrollbar 'collapsed' attribute is changed). This is
going to require some work.

    
    

    
  
Actually, an ugly hack that would work around the crash for now would be to set
|aNotify| to false and then call AttributeChanged directly.  Though really that
isn't too far away from adding an interface and renaming the AttributeChanged
methods on nsSliderFrame and nsNativeScrollbarFrame to something else (probably
more than one method).  Which isn't that far from making it an interface with
getters and setters and not using attributes...

    
    

    
  
But the last wouldn't really work because the scrollbars depend on attributes
because of the way they use XBL.

The current design is nice.  There's a bunch of key stuff in
nsSliderFrame::AttributeChanged, which relies on having a slider inside the
scrollbar that xbl:inherits the maxpos attribute.  That certainly seems like it
should at least be in nsScrollbarFrame instead.  Roughly the same code is in
nsNativeScrollbarFrame::AttributeChanged.  So perhaps it should be in
nsGfxScrollbarFrame -- and doing so would solve the problem mentioned in comment
37 (which I think is the same problem as mentioned in bug 156547 comment 4). 
But that might break the users of scrollbars other than nsGfxScrollFrame.

    
    

    
  
We also have to remove the setting of 'collapsed'. We can probably not bother
collapsing the scrollbar and just explicitly size it to (0,0) in nsGfxScrollFrame.

    
    

    
  
what seems like the 'right' fix to me (if at all possible) is to not let
BeginUpdate add content to the document. It seems like that could be dangerous
for more things then the reflow code.

    
    

    
  
(I wonder if the BeginUpdate is related to some of the content sink / parser
timer stuff.)

In any case, I think changing content during reflow should be forbidden.  Use of
things like CSS attribute selectors can easily cause frame reconstruction, and
selectors that we may want to support in the future could be even more dangerous
than what we have now.

The question is how to do that while continuing to support the XUL scrollbar
"API".  One possibility is that scrollbars could be their own document. 
(Consider bug 21890.  Maybe this makes sense.)

    
    

    
  
letting scrollbars be their own documents sounds expensive. nsDocument is pretty
big and it's getting bigger by the minute.

Could you maybe hold off setting the scrollbar attributes until the reflow is
done by keeping a list off scrollbars that needs to be updated and dealing with
them afterwards?

    
    

    
  
I think we can probably use a special API for nsGfxScrollFrame and XUL listboxes
and trees --- the places where we actually use scrollbars for scrolling, and set
attributes in a delayed event for other XUL consumers (are there any?)

    
    

    
  
But obviously this change would not be a good idea for 1.5

    
    

    
  
re: comment 45, BeginUpdate does not add content to the document.  The content
is already in the document (that is, in the DOM).  BeginUpdate notifies the
document observers that the content is in the document.

Basically, when creating a document via the content sink, we batch up the
ContentAppended notifications for performance reasons.  When BeginUpdate is
called, we flush out whatever notifications we have not delivered yet so that
the various document observers are updated to the then-current actual state of
the DOM.  One of these document observers is the frame constructor (actually the
presshell, but the end result is frame construction), and frame construction can
lead to removal of anonymous content.

    
    

    
  
Why can't we flush those notifications before starting reflow? That would stop
these crashers.

It would not change the fact that changing the DOM during reflow is a bad idea,
though.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        partial fix (obsolete)
        — 
          — Splinter Review
      

    


  
This gets rid of the use of the 'collapsed' attribute. Instead of collapsing
the scrollbars to hide them, we just force their width to be zero during
reflow. This also simplifies the code because we can rely on
scrollbar->GetPrefSize always returning the actual desired width.

This patch actually works pretty well. There are two issues I need to
investigate: an nsPresShell assertion that some frame's desired size changed
during incremental reflow, and the scrollbar on a XUL listbox appears too wide.


This doesn't fix the crashers, of course, because other uses of attributes
remain.

    
    

    
  
Comment on attachment 131163 [details] [diff] [review]
partial fix

Does this work with mac native scrollbars?

    
    

    
  
It should AFAIK. I don't know whether it actually does.

    
    

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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        band-aid fix
        — 
          — Splinter Review
      

    


  
This patch works around the problem by causing notifications to be flushed
before we start reflow. It's nearly zero risk and seems to fix the crashers for
me (although I was never able to reproduce this particular one reliably).

I'll keep working on fixing scrollbars for real.

    
    

    
  
Comment on attachment 131205 [details] [diff] [review]
band-aid fix

I think we should land this ASAP, to make sure it's enough of a fix for 1.5.

        Attachment #131205 -
        Flags: superreview?(dbaron)

        Attachment #131205 -
        Flags: review?(dbaron)

    
    

    
  
it could land on the trunk now that we have branched for 1.5, give it some shake
out therem, and land on the branch if all looks well...
Keywords: topcrash → topcrash+

    
    

    
  
I'll land it on the trunk as soon as it gets reviewed and I get my CVS access
back :-)

        Attachment #131205 -
        Flags: superreview?(dbaron)

        Attachment #131205 -
        Flags: superreview+

        Attachment #131205 -
        Flags: review?(dbaron)

        Attachment #131205 -
        Flags: review+

    
    

    
  
I checked in the bandaid fix for roc

    
    

    
  
I'm continuing work on removing the use of 'collapsed'. The assertion was easy
to get rid of. The listbox scrollbar having the wrong width turned out to be a
very subtle bug that is merely revealed by the 'collapsed' change:

nsIScrollableFrame::GetScrollbarSizes doesn't have a layoutstate parameter, so
it creates a dummy nsBoxLayoutState to use in the call to
scrollbar->GetPrefSize(). The XUL layout code then can't get metrics from the
native theme, because the nsBoxLayoutState doesn't reference an
nsHTMLReflowState, so no rendering context is available. So it falls back to CSS
and gets the wrong size.

Obviously the GetScrollbarSizes interface needs to be changed. My plan is to
split it into two methods: one which doesn't take an nsBoxLayoutState and
returns just the existing actual scrollbar frame sizes (0 if a scrollbar is not
visible) --- most callers just want this, and currently have to work hard to get
it --- and one which takes an nsBoxLayoutState and computes the desired
scrollbar sizes (if they were all visible).

There's also the issue that pretty much everyone who calls GetScrollbarSizes
does the wrong thing if the scrollbars are on the left or top. I'll fix this too.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix as described above (obsolete)
        — 
          — Splinter Review
      

    


  
I'm not sure whether this will work on the Mac. It will size the frame, view
and widget for the Mac native scrollbars to something empty. However, I'm not
sure if the Cocoa widget will do the right thing when it has zero width or
height.

    
    

    
  
It would be great if someone could test this on the Mac.

    
  
Flags: blocking1.5? → blocking1.5+

    
    

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

    
    

    
  
This didn't seem to make it onto the branch. Do we need it there?

    
    

    
  
Yes, we should have the band-aid fix on the branch.

    
    

    
  
Comment on attachment 131205 [details] [diff] [review]
band-aid fix

a=asa (on behalf of drivers) for checkin to the 1.5 branch. Please add the
fixed1.5 keyword when this fix lands on the branch.

        Attachment #131205 -
        Flags: approval1.5? → approval1.5+

    
    

    
  
Comment on attachment 131205 [details] [diff] [review]
band-aid fix

Fix checked in to MOZILLA_1_5_BRANCH, 2003-09-23 15:24 -0700.

    
    

    
  
The Mac browser (trunk) seems to work fine for me now, on the test page, and
subsequent tv guide pages.
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6a) Gecko/20031016

With my old copy (which was several weeks old), I couldn't even load the first
page without it crashing.

    
    

    
  
Stealing.
Assignee: dbaron → roc

    
    

    
  
Comment on attachment 135455 [details] [diff] [review]
updated to trunk

Fix to eliminate some attribute setting in nsGfxScrollFrame::Layout (setting
attributes during layout is eeevil) -- not for 1.6, so no rush

        Attachment #135455 -
        Flags: superreview?(bryner)

        Attachment #135455 -
        Flags: review?(bryner)

    
    

    
  
Javier Pedemonte tested this patch on Mac and says it works there.

    
    

    
  
Comment on attachment 135455 [details] [diff] [review]
updated to trunk

>Index: layout/xul/base/src/nsScrollbarFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/xul/base/src/nsScrollbarFrame.cpp,v
>retrieving revision 1.47
>diff -u -t -p -6 -r1.47 nsScrollbarFrame.cpp
>--- layout/xul/base/src/nsScrollbarFrame.cpp	4 Aug 2003 12:39:40 -0000	1.47
>+++ layout/xul/base/src/nsScrollbarFrame.cpp	14 Nov 2003 03:14:39 -0000
>@@ -97,12 +97,33 @@ nsScrollbarFrame::Init(nsIPresContext*  
>   // scrollbar or change the size of the scrollbar frame.
>   mState |= NS_FRAME_REFLOW_ROOT;
> 
>   return rv;
> }
> 
>+NS_IMETHODIMP
>+nsScrollbarFrame::Reflow(nsIPresContext*          aPresContext,
>+                         nsHTMLReflowMetrics&     aDesiredSize,
>+                         const nsHTMLReflowState& aReflowState,
>+                         nsReflowStatus&          aStatus)
>+{
>+  nsresult rv = nsBoxFrame::Reflow(aPresContext, aDesiredSize, aReflowState, aStatus);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  // nsGfxScrollFrame may have told us to shrink to nothing. If so, make sure our
>+  // desired size agrees.
>+  if (aReflowState.availableWidth == 0) {
>+    aDesiredSize.width = 0;
>+  }
>+  if (aReflowState.availableHeight == 0) {
>+    aDesiredSize.height = 0;
>+  }
>+
>+  return NS_OK;
>+}
>+

Would this change not also be needed in nsNativeScrollbarFrame.cpp?

>Index: layout/xul/base/src/grid/nsGridRowLeafLayout.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/xul/base/src/grid/nsGridRowLeafLayout.cpp,v
>retrieving revision 1.6
>diff -u -t -p -6 -r1.6 nsGridRowLeafLayout.cpp
>--- layout/xul/base/src/grid/nsGridRowLeafLayout.cpp	8 Nov 2001 20:46:02 -0000	1.6
>+++ layout/xul/base/src/grid/nsGridRowLeafLayout.cpp	14 Nov 2003 03:14:45 -0000
>@@ -319,32 +319,30 @@ nsGridRowLeafLayout::ComputeChildSizes(n
>      nsIBox* scrollbox = nsnull;
>      aBox->GetParentBox(&aBox);
>      scrollbox = nsGrid::GetScrollBox(aBox);
>        
>        nsCOMPtr<nsIScrollableFrame> scrollable = do_QueryInterface(scrollbox);
>        if (scrollable) {
>-
>-          // get the clip rect and compare its size to the scrollframe.
>-          // we just need to subtract out the difference. 
>-          nsSize clipSize(0,0);
>-          scrollable->GetClipSize(nsnull, &clipSize.width, &clipSize.height);
>+          nsMargin scrollbarSizes = scrollable->GetActualScrollbarSizes();
> 
>           nscoord diff = 0;
> 
>           nsRect ourRect;
>           nsMargin padding(0,0,0,0);
>           scrollbox->GetBounds(ourRect);
>           scrollbox->GetBorderAndPadding(padding);
>           ourRect.Deflate(padding);
>           scrollbox->GetInset(padding);
>           ourRect.Deflate(padding);
> 
>+          NS_ASSERTION(scrollbarSizes.left == 0 && scrollbarSizes.top == 0,
>+                       "We don't support RTL or weirder scripts here!");

grid doesn't support RTL?

    
    

    
  
> Would this change not also be needed in nsNativeScrollbarFrame.cpp?

Yes, you're right. I'll add it.

> grid doesn't support RTL?

Well... I'm not sure. I'm generally dubious about this chunk of code that deals
with scrollbars; note for example
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/grid/nsGridRowLeafLayout.cpp#363
"last->size -= diff;" --- it appears this could easily give 'last' a negative size.

In PopulateBoxSizes there's code that assumes the first child box is on the left:
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/grid/nsGridRowLeafLayout.cpp#255
so if grids are RTL aware they're not doing a very good job of it.

However, I can make a small improvement to ensure that whatever RTL support
there is isn't damaged:

     if (isHorizontal) {
-      diff = ourRect.width - clipSize.width;
+      diff = ourRect.width - (scrollbarSizes.left + scrollbarSizes.right);
     } else {
-      diff = ourRect.height - clipSize.height;
+      diff = ourRect.height - (scrollbarSizes.top + scrollbarSizes.bottom);
     }

    
    

    
  
Sorry, that would be:

     if (isHorizontal) {
-      diff = ourRect.width - clipSize.width;
+      diff = scrollbarSizes.left + scrollbarSizes.right;
     } else {
-      diff = ourRect.height - clipSize.height;
+      diff = scrollbarSizes.top + scrollbarSizes.bottom;
     }

    
    

    
  
Comment on attachment 135455 [details] [diff] [review]
updated to trunk

Ok, let's do that (r/sr=bryner with those changes).

Fwiw, I know <box> was designed to be RTL aware.  So, grid should be as well,
modulo bugs.

        Attachment #135455 -
        Flags: superreview?(bryner)

        Attachment #135455 -
        Flags: superreview+

        Attachment #135455 -
        Flags: review?(bryner)

        Attachment #135455 -
        Flags: review+

    
    

    
  
I just want to remind that the bandaid fix still is in the trunk (and in 1.6)
and the "right" fix seems to be waiting for checkin, having r= and sr=...

    
    

    
  
Okay, that patch is checked in. But I'm leaving this bug open because we haven't
yet eliminated all uses of SetAttr.

    
    

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

    
    

    
  
Asa:
Please read comments on bug 234404.
blocking1.4.2?
Flags: blocking1.4.2?

    
    

    
  
See bug 234404

Kozawa-san believe that this pageload crash fixed the bug in 234404.

What do people think of this?

Can we get a simple fix for this problem that I can put in 1.4.2?

did this problem happen in 1.4?

    
    

    
  
No, does not crash with 1.4.1/Win32, following procedure in comment 0.
Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.4.1) Gecko/20031008

    
    

    
  
Seems to work for me. Win XP 20040218 Firefox/0.8.0+

    
    

    
  
now it only occurs on:
-> 1.4 Branch
Version: Trunk → 1.4 Branch

    
    

    
  
Um.. could we please not hijack this bug?  See comment 79.
Version: 1.4 Branch → Trunk

    
  
Blocks: 234404

    
  
Flags: blocking1.4.2?

    
    

    
  
WFM.

- Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a) Gecko/20040512
Firefox/0.8.0+
- Microsoft Windows 2000 Pro 5.00.2195 SP4

    
  
Summary: pageload crash [nsXULElement::SetAttr] → pageload crash [@ nsXULElement::SetAttr]

    
    

    
  
(In reply to comment #79)
> Okay, that patch is checked in. But I'm leaving this bug open because we
> haven't yet eliminated all uses of SetAttr.

Roc, has this been done yet? Can this bug be closed?



    
    

    
  
Yes, most of those SetAttrs that are dangerous have been worked around one way or another. And this should not be a tracking bug.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsXULElement::SetAttr]

    
  
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: