236947 - Camino hangs on product selection (list) in bugzilla query.

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

[Hiro]

If a product is chosen on a bugzilla query, Camino will hang-up.
In addition, there is no problem in OSX version firefox nightly build.


Reproducible: Always
Steps to Reproduce:
1.Open bugzilla query.
2.Arbitrary products are chosen from a product list.
  e.g. select Camno

Actual Results:
Camino hangs-up.

Expected Results:
Camino does not hang-up.
The component relevant to the selected product etc. is displayed.

Mac OS X 10.3.2 and New Profile
2004030808 (v0.7+)

Comment 1

[Mike Pinkerton (not reading bugmail)]

sigh. what date did this start happening. wonder if it's related to the rtl
style i put on selects (though i don't see how).

Comment 2

[Hiro]

Even if it chooses arbitrary products, the list of the component relevant to the
selected product, target, etc. does not change.
And cursor changes to rainbow cursor after several seconds.
Then, a reaction nothing....

Comment 3

[Hiro]

An additional comment:
There was no problem in build of the following.

2004030508 (v0.7+)
2004030708 (v0.7+)

Comment 4

[Jasper]

I can confirm that the hanging started on the 20040307 NB. I can't find any
checkins that have been done that might cause this behaviour. Somebody else
might have to take a look.

Comment 5

[Jasper]

It's not the rtl style you added to the select widget. I tested it by removing
it fro the platform-forms.css.

Comment 6

[Hiro]

Attachment: crash log

Crash occurred only once.
And the problem was solved, when this log was made reference and the patch of
the following bugs was removed.
   bug 51938
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=Chimera&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=03%2F07%2F2004+10%3A04&maxdate=03%2F07%2F2004+10%3A04&cvsroot=%2Fcvsroot

Comment 7

[Mike Pinkerton (not reading bugmail)]

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

Comment 8

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

Hmm. It would be really helpful to know what is at
"nsBlockFrame::GetFirstLineContaining(int) + 0x90". Someone with OSX and a
debugger could probably find out...

Comment 9

[Stuart Morgan]

I did this a few times with the debugger, and both times I got basically the
same result.  It gets to the 
first while loop of nsBlockFrame::GetFirstLineContaining with y=0:

  while ((cursorArea.IsEmpty() || cursorArea.YMost() > y)
         && cursor != mLines.front()) {
    cursor = cursor.prev();
    cursorArea = cursor->GetCombinedArea();
  }

Then it gets into the following cycle:
iteration n:
    cursorArea.IsEmpty() = 1
    cursor = <cursor_1_value>
    cursor.prev() = <cursor_2_value>
iteration n+1:
    cursorArea.IsEmpty() = 0
    cursorArea.YMost() = <a really big number>
    cursor = <cursor_2_value>
    cursor.prev() = <cursor_1_value>

<really big number> varied somewhat between each time I did the debugging; once
it was 68567554, the next time 68410882


Once I did this by pausing once it was already hung, so I didn't see the
starting iterations. The next time though I managed to pause before it hit this
function, and the very first iteration of the loop on the call that hung it was
the IsEmpty=1 iteration I listed first.

Comment 10

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

Errr

What is/was mLines.front()?

We should have stopped as soon as we reached the first line. If we didn't see
the first line, then something is very wrong.

Comment 11

[Stuart Morgan]

Values from another trial of the hung call:

mLines.begin(property)        = 0x41e0eac
  corresponding IsEmpty: true
mLines.begin(property).next() = 0x4210500
mLines.begin(property).prev() = 0x4210500
  corresponding IsEmpty: false
    corresponding YMost: 69182466

The prev() pointer for 0x4210500 is back to 0x41e0eac

mLines.front()                = 0x4210558
mLines.back()                 = 0x41e11ac
mLines.size()                 = 16


I admit I haven't had time to really dig into the code, so I don't know what
mLines is supposed to be/do (so I apologize if I'm not pulling all the important
info here)... but I would imagine that a list structure reporting its size as 16
shouldn't have a circular structure of size 2 in it.

Comment 12

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

Owwww. The cursor is pointing to some lines that aren't part of this line array.
Maybe they're deleted lines, or maybe they're lines belonging to some other block.

This sounds like a job for Purify or something equivalent. Do we have anything
like that on the Mac?

Comment 13

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

(thanks for the help, Stuart)

Comment 14

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

Actually Stuart, if you want to be even more helpful, you could get a frame dump
of the guilty document (using Tools ... Layout Debugger), then trigger the bug
and in the debugger, look at the mFirstChild field of each of the lines and
report all that along with the frame dump.

Comment 15

[Hiro]

This problem was reproduced also by firefox.
Probably, it may reproduce also by mozilla.

Steps to Reproduce:
1.Open bugzilla query.
2.Arbitrary products are chosen from a product list.
  e.g. select Camno
3.Execute search
  a search result is displayed
4.The Go Back one page button is pushed.

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; ja-JP; rv:1.7b) Gecko/20040312
Firefox/0.8.0+

Comment 16

[Hiro]

(In reply to comment #15)
> 4.The Go Back one page button is pushed.
It reproduces, even if it clicks the link of "Edit Search".

Comment 17

[Boris Zbarsky [:bzbarsky]]

I've been hitting this on Linux too, with various nightlies. This is the #1
cause of Mozilla dataloss for me over the last several days.

Comment 18

[Boris Zbarsky [:bzbarsky]]

This is totally dogfood.

Comment 19

[Boris Zbarsky [:bzbarsky]]

OK.  So looking at the blockframe code, the code in nsBlockFrame::DoRemoveFrame
looks suspicious.  This can remove a line, and then end up posting both an
invalidate and a reflow event.  If the paint is done before the reflow, we get
screwed, no?

Comment 20

[Boris Zbarsky [:bzbarsky]]

And in fact, if I set the cursor to null right after that erase() call, I get a
nice satisfying dereferencing-a-null-pointer crash when I click about the query
page, instead of hanging.

Comment 21

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

Oh yeah, there should *definitely* be a call to ClearLineCursor in there.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: This should fix it...

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143845 [details] [diff] [review]
This should fix it...

Lemme know if you think we should compare line to the cursor (only if we have
the bit set, of course).  It's dead-simple to, of course.

Comment 24

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

Actually, I think for sanity's sake we should call ClearLineCursor right at the
top of DoRemoveFrame. The cursor should be cleared not only if we delete a line
but also if we could disturb the "lines' combinedArea.y/ymosts are
non-decreasing" invariant. I don't *think* DoRemoveFrame can disturb that, but
why worry about it?

For the same reason I think we should call ClearLineCursor at the top of
AddFrames too.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Why?  Any of those operations will trigger a reflow, which will clear the cursor
and all that.  The only reason we need to clear the cursor here is so that if we
get a repaint or mouse move before a reflow we don't point to a bogus
linebox....  Or are you worried about transient rendering stuff?  I think
worrying about what happens between the moment a frame is removed from the frame
tree and the moment when we process the reflow command is a bit moot from a
rendering correctness perspective.

Comment 26

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

> Or are you worried about transient rendering stuff?

That's an issue, but I'm more worried about transient event handling.

But I'm most worried about simple maintainable code, and I think the simplest
most maintainable policy is to call ClearLineCursor whenever you alter the lines
or their geometry. I *think* that your patch is correct (AddFrames isn't really
a problem because any added lines should have empty combined areas until Reflow,
so don't participate in the cursor invariant) --- but I don't think it's worth
having to think about this, now or in the future. I can just see someone
modifying AddFrames or DoRemoveFrame and breaking this.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Attachment: OK, I buy that

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143846 [details] [diff] [review]
OK, I buy that

We should land this for beta...  Hangs are bad and all.

Comment 29

[chris hofmann]

Comment on attachment 143846 [details] [diff] [review]
OK, I buy that

a=chofmann for 1.7b

Comment 30

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 31

[:Mook]

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

Comment 32

[HARUNAGA Hirotoshi]

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

Comment 33

[WD]

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

Comment 34

[Stuart Morgan]

20040315 NB of Camino is verified working