629114 - crash on add comment link in review board [@ nsBulletFrame::GetListItemText ] [@ nsBulletFrame::GetListItemText(nsStyleList const&, nsString&) ]

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Olen.E.Boydstun]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b10) Gecko/20100101 Firefox/4.0b10
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b10) Gecko/20100101 Firefox/4.0b10

goto the review board demo page, make yourself an account.
Select any code review, and look for an "Add comment" link.
Click in Firefox beta 4.0 and firefox will crash on linux (32 bit)


Reproducible: Always

Steps to Reproduce:
1.Select add comment to reply to comment in a code review
2.using firefox 4 beta on linux 32 bit
3.
Actual Results:  
firefox crashed, and tries to send a crash report.  It does this everytime.  Apparently these crash reports aren't getting turned into bugzilla issues, since no issue mentioning compatibility with reviewboard is found in the database.

Expected Results:  
to be apply to reply to the reviewer's comments.

Comment 1

[Tyler Downer [He/Him]]

https://developer.mozilla.org/En/How_to_get_a_stacktrace_for_a_bug_report

Comment 2

[Olen.E.Boydstun]

Here are some stack trace ID'sSubmitted Crash Reports
    
    bp-77ce086c-d47d-4fa0-9ca2-129dd2110126 
    bp-b0eef424-14a6-465e-a95b-600d92110126
    bp-a8840bbd-0796-4095-9c80-9e42b2110125

Comment 3

[Matthias Versen [:Matti]]

0 	libxul.so 	nsBulletFrame::GetListItemText 	layout/style/nsStyleStructList.h:103
1 	libxul.so 	nsBlockFrame::GetBulletText 	layout/generic/nsBlockFrame.cpp:6634
2 	libxul.so 	nsHTMLListBulletAccessible::GetName 	accessible/src/html/nsHTMLTextAccessible.cpp:367
3 	libxul.so 	nsTextEquivUtils::AppendFromAccessible 	accessible/src/base/nsTextEquivUtils.cpp:237
4 	libxul.so 	nsTextEquivUtils::AppendFromAccessibleChildren 	accessible/src/base/nsTextEquivUtils.cpp:217
5 	libxul.so 	nsTextEquivUtils::AppendFromAccessible 	accessible/src/base/nsTextEquivUtils.cpp:260
6 	libxul.so 	nsTextEquivUtils::AppendFromAccessibleChildren 	accessible/src/base/nsTextEquivUtils.cpp:217
7 	libxul.so 	nsTextEquivUtils::AppendFromAccessible 	accessible/src/base/nsTextEquivUtils.cpp:260
8 	libxul.so 	nsTextEquivUtils::AppendFromAccessibleChildren 	accessible/src/base/nsTextEquivUtils.cpp:217
9 	libxul.so 	nsTextEquivUtils::GetNameFromSubtree 	accessible/src/base/nsTextEquivUtils.cpp:72
10 	libxul.so 	nsAccessible::GetHTMLName 	accessible/src/base/nsAccessible.cpp:1160
11 	libxul.so 	nsAccessible::GetName 	accessible/src/base/nsAccessible.cpp:244
12 	libxul.so 	getNameCB 	accessible/src/atk/nsAccessibleWrap.cpp:709
13 	libatk-1.0.so.0.1212.0 	libatk-1.0.so.0.1212.0@0xa948

Comment 4

[chris hofmann]

can anyone provide a link to the "review board demo page"?

Comment 5

[Olen.E.Boydstun]

(In reply to comment #4)
> can anyone provide a link to the "review board demo page"?

Its the URL above, http://demo.reviewboard.org.   You do have to make a test account for yourself, but then use any test review, and try to add a comment, and on my 32-bit RHEL5 box, the crash happens with the firefox 4 betas.  Haven't tried it with a windows version.

Comment 7

[Mats Palmgren (inactive)]

bp-2f8d49fd-269f-49a1-bfc6-913162110304

Comment 8

[Rashmi Ranjan Mohanty]

(In reply to comment #6)
> *** Bug 638006 has been marked as a duplicate of this bug. ***

Observed a very strange behavior for bug 638006. I can reproduce the bug only on laptop with SLED 11 SP1. Even on desktops with SLED11 SP1 do not reproduce the issue.

Comment 9

[alexander :surkov (:asurkov)]

Steps to reproduce on Windows: go to http://www.w3.org/TR/wai-aria-practices/ while NVDA or JAWS running.

The stack:

 	xul.dll!nsIFrame::GetStyleVisibility()  Line 103 + 0xa bytes	C++
 	xul.dll!nsBulletFrame::GetListItemText(const nsStyleList & aListStyle, nsString & result)  Line 1255 + 0x8 bytes	C++
 	xul.dll!nsBlockFrame::GetBulletText(nsAString_internal & aText)  Line 6634	C++
 	xul.dll!nsHTMLListBulletAccessible::GetName(nsAString_internal & aName)  Line 370	C++
 	xul.dll!nsTextEquivUtils::AppendFromAccessible(nsAccessible * aAccessible, nsAString_internal * aString)  Line 237 + 0x19 bytes	C++
 	xul.dll!nsTextEquivUtils::AppendFromAccessibleChildren(nsAccessible * aAccessible, nsAString_internal * aString)  Line 217 + 0xd bytes	C++
 	xul.dll!nsTextEquivUtils::AppendFromAccessible(nsAccessible * aAccessible, nsAString_internal * aString)  Line 260 + 0xd bytes	C++
 	xul.dll!nsTextEquivUtils::AppendFromAccessibleChildren(nsAccessible * aAccessible, nsAString_internal * aString)  Line 217 + 0xd bytes	C++
>	xul.dll!nsTextEquivUtils::AppendFromAccessible(nsAccessible * aAccessible, nsAString_internal * aString)  Line 260 + 0xd bytes	C++
 	xul.dll!nsTextEquivUtils::AppendFromAccessibleChildren(nsAccessible * aAccessible, nsAString_internal * aString)  Line 217 + 0xd bytes	C++
 	xul.dll!nsTextEquivUtils::GetNameFromSubtree(nsAccessible * aAccessible, nsAString_internal & aName)  Line 72 + 0x10 bytes	C++
 	xul.dll!nsAccessible::GetHTMLName(nsAString_internal & aLabel)  Line 1156 + 0x10 bytes	C++
 	xul.dll!nsAccessible::GetNameInternal(nsAString_internal & aName)  Line 2702 + 0xc bytes	C++
 	xul.dll!nsAccessible::GetName(nsAString_internal & aName)  Line 244 + 0x15 bytes	C++
 	xul.dll!nsAccessibleWrap::get_accName(tagVARIANT varChild, wchar_t * * pszName)  Line 280 + 0x1f bytes	C++

request blocking (at least 2.x blocking) - no way to navigate certain web pages for blinds.

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Is this a regression? Would be good to get a range.

Comment 11

[alexander :surkov (:asurkov)]

the problem of nsBlockFrame::mBullet is null when GetBulletText is called, the same time, nsBlockFrame::IsBulletEmpty() returned false earlier. It's either IsBulletEmpty() may return false but mBullet is null (the problem in nsBlockFrame methods) or the frame tree were changed between IsBulletEmpty() and GetBulletText() calls.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> Is this a regression?

Sure it is, fx3.6 doesn't crash.

Comment 13

[David Bolter [:davidb] (NeedInfo me for attention)]

I don't understand why calling GetStyleVisibility() crashes -- or is a bad stack?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> I don't understand why calling GetStyleVisibility() crashes -- or is a bad
> stack?

I don't think it's bad stack.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> I don't understand why calling GetStyleVisibility() crashes

Because you're calling it on a null pointer, per comment 11?

BulletIsEmpty() is clearly documented as something you should only call when there is a bullet, right?

In particular only when display is list-item and HaveOutsideBullet(), though it looks like a11y is violating this last for sure.

The simplest way to have a null mBullet here is to be ending up in the callstack of comment 9 when the block is not display:list-item.  Does a11y create the list-item accessible based on the tag name instead of the display type or something?

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks Boris (I'm traveling and about about to pass out). I think Alexander is on this though.

Comment 17

[alexander :surkov (:asurkov)]

Attachment: testcase

crash is on dt listitem

Comment 18

[alexander :surkov (:asurkov)]

(In reply to comment #15)

> BulletIsEmpty() is clearly documented as something you should only call when
> there is a bullet, right?

perhaps not so clearly, but documented, I've got usual to see explicit warnings.

>  Does a11y
> create the list-item accessible based on the tag name instead of the display
> type or something?

from case to case. but yes, for dt elements we create list item accessible based on tag name.

Comment 19

[alexander :surkov (:asurkov)]

Attachment: testcase

no screen reader required

Comment 20

[Boris Zbarsky [:bzbarsky]]

> for dt elements we create list item accessible based on tag name.

Yeah, then you need to be doing some manual style checks on the block to make sure it has a bullet before asking it for information about the bullet.

Comment 21

[alexander :surkov (:asurkov)]

(In reply to comment #20)
> > for dt elements we create list item accessible based on tag name.
> 
> Yeah, then you need to be doing some manual style checks on the block to make
> sure it has a bullet before asking it for information about the bullet.

Yes. Does bullet frame existence guarantee me there's not empty bullet text? Can bullet frame be created/destroyed during the block frame life cycle?

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Does bullet frame existence guarantee me there's not empty bullet text?

No.  See list-style-type:none.

> Can bullet frame be created/destroyed during the block frame life cycle?

Effectively, no.  It's created pretty late during the construction of the block frame, but that's the only place it's created, and it's never destroyed without destroying the block frame itself.

One other thing: in a columns setup, only the first block in the columnset will have a bullet.  In case that matters.  I don't know how a11y picks the block frame to look at.

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #22)

> One other thing: in a columns setup, only the first block in the columnset will
> have a bullet.  In case that matters.  I don't know how a11y picks the block
> frame to look at.

Is this about the case?
<dl>
<dt>item1</dt><dd>definition1</dd>
<dt>item2</dt><dd>definition2</dd>
</dl>

Is 'first block in the columnset' a frame of first dt element? Does "have a bullet" means the block frame has a bullet frame?

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Is this about the case?

No, it's about the case:

  <ol>
    <li style="-moz-column-count: 2; width: 100px;">Some text here</li>
  </ol>

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm sorry, can I get a concise explanation of why we shouldn't ship Firefox 4 with this bug in it? Or is the blocking nomination here to pick up .x?

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #25)
> I'm sorry, can I get a concise explanation of why we shouldn't ship Firefox 4
> with this bug in it? Or is the blocking nomination here to pick up .x?

I'm not sure whether it should block Firefox 4 or not, perhaps .x works here. What is criteria for blockers? For example, is it ok to ship the browser that crashes when web pages with certain markup loaded? It's not very often used markup, but is used for example on w3c pages.

Comment 27

[David Bolter [:davidb] (NeedInfo me for attention)]

If there is a no (low) risk fix we can do here (even taking a loss in a11y accuracy to avoid the crash) an RC ride-along is the goal I think.

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 29

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> > Is this about the case?
> 
> No, it's about the case:
> 
>   <ol>
>     <li style="-moz-column-count: 2; width: 100px;">Some text here</li>
>   </ol>

I don't get bullet frame for li's blockFrame. Is the frame tree like
block frame - li
  block frame - col1
    bullet frame
  block frame - col2

i.e the bullet frame belongs to block frame of first column? If so what the best way to find that blockFrame?

Comment 30

[Boris Zbarsky [:bzbarsky]]

> I don't get bullet frame for li's blockFrame.

In what sense?

> Is the frame tree

You do know about the layout debugger extension, right?

The frame tree in this case is a columnset frame for the li, which contains two block frames.  The first block frame has a bullet.  The primary frame for the <li> is the columnset.

> If so what the best way to find that blockFrame?

Good question.

Comment 31

[Boris Zbarsky [:bzbarsky]]

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

This now creates an accessible even if there is no bullet text, right?  Is that desired?

Comment 32

[alexander :surkov (:asurkov)]

(In reply to comment #31)
> Comment on attachment 519692 [details] [diff] [review]
> patch
> 
> This now creates an accessible even if there is no bullet text, right?  Is that
> desired?

ideally we don't want an accessible if there's no bullet text, but if the bullet text is changed from empty to not empty then I afraid we won't update an accessible tree since there's no frame tree changes. Perhaps I should deal with that in followup.

Comment 33

[Sheila Mooney]

At #226 in the top 300 for RC with 96 crashes over the past 3 days.

Comment 34

[David Bolter [:davidb] (NeedInfo me for attention)]

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

This looks like it should fix the crash since the new GetBulletFrame simply returns mBullet. I don't see any risk here, but Boris would know better.

We'll need a try run of course.

Comment 35

[Boris Zbarsky [:bzbarsky]]

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

I would prefer a boolean HasBulletFrame() to something that hands out the bullet frame itself.

sr=me with that changed.

Note that changes to list-style-type _do_ trigger reflow, by the way.

Comment 36

[David Bolter [:davidb] (NeedInfo me for attention)]

Note this site causes the crash: http://www.w3.org/TR/wai-aria-practices/
A little embarrassing, but that's life. I can reach out to have that site tweaked.

Comment 37

[David Bolter [:davidb] (NeedInfo me for attention)]

Would be good to get QA on this with a tablet PC.

STR: use tablet PC to go to the site in comment 36.

I suspect it might be immune since that software shouldn't need the accessible name for its purposes.

Comment 38

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> Comment on attachment 519692 [details] [diff] [review]
> patch
> 
> I would prefer a boolean HasBulletFrame() to something that hands out the
> bullet frame itself.

ok

> Note that changes to list-style-type _do_ trigger reflow, by the way.

How can I use it?

Comment 39

[Boris Zbarsky [:bzbarsky]]

> How can I use it?

I thought you already updated some accessibles after reflow, no?  Could you not do the same here?

Comment 40

[alexander :surkov (:asurkov)]

(In reply to comment #39)
> > How can I use it?
> 
> I thought you already updated some accessibles after reflow, no?

Accessible tree update is scheduled on content insertion (nsCSSFrameConstructor::ContentRangeInserted and etc) and on text reflow (nsTextFrame::ReflowText()).

>  Could you not
> do the same here?

That's worth idea. Then I'd need to add a11y notification to update tree when list type of bullet is changed. What's the right place for that?

Comment 41

[Boris Zbarsky [:bzbarsky]]

I'm not sure offhand.  Perhaps in the bullet code if it changes its text?

Comment 42

[alexander :surkov (:asurkov)]

(In reply to comment #41)
> I'm not sure offhand.  Perhaps in the bullet code if it changes its text?

perhaps nsBulletFrame::DidSetStyleContext?

Comment 43

[Boris Zbarsky [:bzbarsky]]

That would work, but you don't care when the _style_ changes so much as when the _text_ changes, right?   The former can happen a bunch of times for each instance of the latter.

Comment 44

[alexander :surkov (:asurkov)]

(In reply to comment #43)
> That would work, but you don't care when the _style_ changes so much as when
> the _text_ changes, right?   The former can happen a bunch of times for each
> instance of the latter.

yes, sure, but primary I care about the case when there's a text or there's no text, that should means I need to catch the change when list type is changed from none to not none (taking into account the bullet frame lifetime equals to its containing block frame).

Comment 45

[alexander :surkov (:asurkov)]

Boris, where should add a hook to notify ally if DidSetStyleContext isn't proper place?

Comment 46

[Boris Zbarsky [:bzbarsky]]

I guess we don't store the text anywhere, huh?  I guess DidSetStyleContext is the simplest thing, then.

Comment 47

[alexander :surkov (:asurkov)]

(In reply to comment #46)
> I guess we don't store the text anywhere, huh?

sounds so.

>  I guess DidSetStyleContext is
> the simplest thing, then.

Ok, I'll go with it.

Comment 48

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 49

[alexander :surkov (:asurkov)]

(In reply to comment #48)
> Created attachment 520889 [details] [diff] [review]
> patch2

I tempted to remove nsHTMLLIAccessible::Shutdown() entirely, mBullet should be nulled though, I got back this locally.

Comment 50

[Marco Zehe (:MarcoZ)]

Comment on attachment 520889 [details] [diff] [review]
patch2

>-      testAccessibleTree("list4", nestedAccTree);

Why was this removed? You build up the tree above. Was this unintentional?

r=me with the above explained/fixed.

Comment 51

[alexander :surkov (:asurkov)]

(In reply to comment #50)
> Comment on attachment 520889 [details] [diff] [review]
> patch2
> 
> >-      testAccessibleTree("list4", nestedAccTree);
> 
> Why was this removed? You build up the tree above. Was this unintentional?

yep, thanks for the catch!

Comment 52

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 520889 [details] [diff] [review]
patch2

r=me. Seems like an improvement to me.

>+void
>+nsHTMLLIAccessible::UpdateBullet(bool aHasBullet)
>+{
>+  if (aHasBullet == !!mBullet) {
>+    NS_NOTREACHED("Bullet and accessible are in sync already!");
>+    return;
>+  }
>+
>+  nsDocAccessible* document = GetDocAccessible();
>+  if (aHasBullet) {
>+    mBullet = new nsHTMLListBulletAccessible(mContent, mWeakShell);
>+    if (document->BindToDocument(mBullet, nsnull))
>+      InsertChildAt(0, mBullet);
>+
>+  } else {

Nit: I balked here. I'd prefer removal of the extra blank line and the addition of {}'s around the if block.

I would never block r+ on this though.

>+  // XXXtodo: fire show/hide and reorder events. That's hard to make it
>+  // right now because coalescence happens by DOM node.

Probably not that important either.

Comment 53

[alexander :surkov (:asurkov)]

(In reply to comment #52)

> Probably not that important either.

it should be important, to make AT to update their vb.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 520889 [details] [diff] [review]
patch2

sr=me

Comment 55

[alexander :surkov (:asurkov)]

landed - http://hg.mozilla.org/mozilla-central/rev/a3bc845d5f47

Comment 56

[chris hofmann]

did this make it to 4.0.1?

Comment 57

[Boris Zbarsky [:bzbarsky]]

Not so far, no.  If we think it should, we should ask for approval on the patch.

Comment 58

[Marco Zehe (:MarcoZ)]

As a follow-up:
Another page that reproduces this bug in Firefox 4, which was reported to me by a user, is this:
http://www.w3.org/WAI/PF/aria-practices/

Just loading this page with Firefox 4 and NVDA causes this crash to happen. I confirmed that this is fixed in 5.0b1.