Closed Bug 629114 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Disability Access APIs, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla5
Tracking Status
blocking2.0 --- .x+
status2.0 --- ?

People

(Reporter: Olen.E.Boydstun, Assigned: surkov)

References

()

Details

(4 keywords)

Crash Data

Attachments

(3 files, 1 obsolete file)

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.
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
Component: General → Layout
Keywords: crash
Product: Firefox → Core
QA Contact: general → layout
Summary: crash on add comment link in review board → crash on add comment link in review board [@ nsBulletFrame::GetListItemText ]
can anyone provide a link to the "review board demo page"?
(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.
Duplicate of this bug: 638006
bp-2f8d49fd-269f-49a1-bfc6-913162110304
Status: UNCONFIRMED → NEW
Component: Layout → Disability Access APIs
Ever confirmed: true
QA Contact: layout → accessibility-apis
Summary: crash on add comment link in review board [@ nsBulletFrame::GetListItemText ] → crash on add comment link in review board [@ nsBulletFrame::GetListItemText ] [@ nsBulletFrame::GetListItemText(nsStyleList const&, nsString&) ]
(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.
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.
blocking2.0: --- → ?
Is this a regression? Would be good to get a range.
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.
(In reply to comment #10)
> Is this a regression?

Sure it is, fx3.6 doesn't crash.
Keywords: regression
I don't understand why calling GetStyleVisibility() crashes -- or is a bad stack?
(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.
> 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?
Thanks Boris (I'm traveling and about about to pass out). I think Alexander is on this though.
Assignee: nobody → surkov.alexander
Attached file testcase (obsolete) —
crash is on dt listitem
(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.
Attached file testcase
no screen reader required
Attachment #519601 - Attachment is obsolete: true
Keywords: testcase
> 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.
(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?
> 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.
(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?
> 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'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?
(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.
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.
Attached patch patchSplinter Review
Attachment #519692 - Flags: superreview?(bzbarsky)
Attachment #519692 - Flags: review?(bolterbugz)
(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?
Flags: in-testsuite?
> 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 on attachment 519692 [details] [diff] [review]
patch

This now creates an accessible even if there is no bullet text, right?  Is that desired?
(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.
At #226 in the top 300 for RC with 96 crashes over the past 3 days.
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.
Attachment #519692 - Flags: review?(bolterbugz) → review+
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.
Attachment #519692 - Flags: superreview?(bzbarsky) → superreview+
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.
Whiteboard: [has patch][proposed ride-along]
blocking2.0: ? → .x+
Whiteboard: [has patch][proposed ride-along] → [has patch][fx4-rc-ridealong]
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.
Keywords: qawanted
(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?
> How can I use it?

I thought you already updated some accessibles after reflow, no?  Could you not do the same here?
(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?
I'm not sure offhand.  Perhaps in the bullet code if it changes its text?
(In reply to comment #41)
> I'm not sure offhand.  Perhaps in the bullet code if it changes its text?

perhaps nsBulletFrame::DidSetStyleContext?
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.
(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).
Boris, where should add a hook to notify ally if DidSetStyleContext isn't proper place?
I guess we don't store the text anywhere, huh?  I guess DidSetStyleContext is the simplest thing, then.
(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.
Attached patch patch2Splinter Review
Attachment #520889 - Flags: superreview?(bzbarsky)
Attachment #520889 - Flags: review?(marco.zehe)
Attachment #520889 - Flags: review?(bolterbugz)
(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 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.
Attachment #520889 - Flags: review?(marco.zehe) → review+
(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 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.
Attachment #520889 - Flags: review?(bolterbugz) → review+
(In reply to comment #52)

> Probably not that important either.

it should be important, to make AT to update their vb.
Whiteboard: [has patch][fx4-rc-ridealong] → [has patch][fx4-rc-ridealong][not-ready]
Comment on attachment 520889 [details] [diff] [review]
patch2

sr=me
Attachment #520889 - Flags: superreview?(bzbarsky) → superreview+
landed - http://hg.mozilla.org/mozilla-central/rev/a3bc845d5f47
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][fx4-rc-ridealong][not-ready]
Target Milestone: --- → mozilla2.2
did this make it to 4.0.1?
status2.0: --- → ?
Not so far, no.  If we think it should, we should ask for approval on the patch.
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.
Crash Signature: [@ nsBulletFrame::GetListItemText ] [@ nsBulletFrame::GetListItemText(nsStyleList const&, nsString&) ]
Crash Signature: [@ nsBulletFrame::GetListItemText ] [@ nsBulletFrame::GetListItemText(nsStyleList const&, nsString&) ] → [@ nsBulletFrame::GetListItemText ] [@ nsBulletFrame::GetListItemText(nsStyleList const&, nsString&) ]
You need to log in before you can comment on or make changes to this bug.