615958 - crash [@ nsTreeBodyFrame::PrefillPropertyArray]

Status: RESOLVED INCOMPLETE

(Core :: XUL, defect, P5)

Description

[Wayne Mery (:wsmwk)]

crash [@ nsTreeBodyFrame::PrefillPropertyArray(int, nsTreeColumn*)]

bp-02f4c420-91a3-4e5e-8f34-c0fea2101109 (arnold9) Printer hp pssc 2410xi photosmart all-in-one wont print
EXCEPTION_ACCESS_VIOLATION_READ
0x0
0	thunderbird.exe	nsTreeBodyFrame::PrefillPropertyArray	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:1991
1	thunderbird.exe	nsTreeBodyFrame::GetItemWithinCellAt	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:1532
2	thunderbird.exe	nsTreeBodyFrame::GetCellAt	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:1708
3	thunderbird.exe	nsTreeBodyFrame::GetCursor	layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:2554
4	thunderbird.exe	nsEventStateManager::UpdateCursor	content/events/src/nsEventStateManager.cpp:3379
5	thunderbird.exe	nsEventStateManager::PreHandleEvent	content/events/src/nsEventStateManager.cpp:1131
6	thunderbird.exe	PresShell::HandleEventInternal	layout/base/nsPresShell.cpp:6508
7	thunderbird.exe	PresShell::HandlePositionedEvent	layout/base/nsPresShell.cpp:6364
8	thunderbird.exe	PresShell::HandleEvent	layout/base/nsPresShell.cpp:6228
9	thunderbird.exe	nsViewManager::HandleEvent	view/src/nsViewManager.cpp:1224
10	thunderbird.exe	nsViewManager::DispatchEvent	view/src/nsViewManager.cpp:1203
11	thunderbird.exe	HandleEvent	view/src/nsView.cpp:167

Comment 1

[timeless]

mView is null, and it's often null and usually null checked...

Comment 2

[timeless]

1967 nsTreeBodyFrame::PrefillPropertyArray(PRInt32 aRowIndex, nsTreeColumn* aCol)
1968 {
1969  NS_PRECONDITION(!aCol || aCol->GetFrame(this), "invalid column passed");

we should crash here if mScratchArray is null:
1970  mScratchArray->Clear();
1971  
1972  // focus
1973  if (mFocused)
1974    mScratchArray->AppendElement(nsGkAtoms::focus);
1975
1976  // sort
1977  PRBool sorted = PR_FALSE;

we should crash here if mView is null:
1978  mView->IsSorted(&sorted);

isSorted() can be js, at which point all assumptions we've made to this point cease to be valid.

1979  if (sorted)
1980    mScratchArray->AppendElement(nsGkAtoms::sorted);
1981
1982  // drag session
1983  if (mSlots && mSlots->mDragSession)
1984    mScratchArray->AppendElement(nsGkAtoms::dragSession);
1985
1986  if (aRowIndex != -1) {

we crashed somewhere after this line:
1987    if (aRowIndex == mMouseOverRow)
1988      mScratchArray->AppendElement(nsGkAtoms::hover);
1989  
1990    nsCOMPtr<nsITreeSelection> selection;
1991    mView->GetSelection(getter_AddRefs(selection));
and not later than this line.

But we've already crashed earlier for both.

I think that we probably need to recheck mView and friends after 1978.

Comment 3

[Yuhong Bao]

BTW, the JavaScript debugger often causes this crash.

Comment 4

[timeless]

Attachment: patch

Comment 5

[[e-mail]]

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

The only way that mScratchArray can disappear is if we are destroyed in which case testing for mScratchArray is the least of our worries.

Comment 6

[[e-mail]]

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

All the consumers of PrefillPropertyArray immediately hit mView so this will just move the crash.

Comment 7

[timeless]

Attachment: mView guards

Comment 8

[Yuhong Bao]

(In reply to comment #3)
> BTW, the JavaScript debugger often causes this crash.
Actually, it doesn't. It crashes at a different location. I may file a new bug later.

Comment 9

[Neil Deakin]

Comment on attachment 495383 [details] [diff] [review]
mView guards

>@@ -1056,8 +1060,10 @@ nsresult
> nsTreeBodyFrame::GetCellAt(PRInt32 aX, PRInt32 aY, PRInt32* aRow, nsITreeColumn** aCol,
>                            nsACString& aChildElt)
> {
>-  if (!mView)
>+  if (!mView) {
>+    *aRow = -1;
>     return NS_OK;

Set the other out parameters as well?

For much of the rest of the checks, it would seem clearer to check mView for null before using it, rather than after it

Comment 10

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

Comment on attachment 495383 [details] [diff] [review]
mView guards

r+ before a?, please

Comment 11

[Wayne Mery (:wsmwk)]

different thunderbird users report trying to access/use addons
bp-56ee7f0a-afff-4d9e-85f1-c92682120416
bp-cebcef33-8261-49e5-bf69-b573e2120410
bp-54c3bca6-03fb-435b-8bdf-555d92120319
bp-1e49ddba-d095-4f2d-aa0e-aa0652120127
bp-5e98773f-ff5b-4837-98d5-98fb32120120

but it's still la small percentage of crashes that mentions addons
And there are way more Thunderbird crashes than FFirefox

Comment 12

[Wayne Mery (:wsmwk)]

highest percentage of comments cites using calendar tabs.

Comment 13

[Ludovic Hirlimann [:Usul]]

Neil Ping review

Comment 14

[Scoobidiver (away)]

It's #204 top crasher in TB 17.0.

Comment 15

[Timothy Nikkel (:tnikkel)]

Attachment: rebased

Saving this here. Just rebased to apply to current. No other changes.

Comment 16

[Wayne Mery (:wsmwk)]

Timothy, 
Thanks for the rebased patch.
Had you seen this crash?
Do you have an opionion regarding viability of the patch?

Comment 17

[Timothy Nikkel (:tnikkel)]

I don't remember what prompted me to rebase the patch. I don't think I saw the crash. Maybe I was trying to fix some intermittent failures in automation or something I found in crash-stats.

Patch might be a little overkill. If we could add the null checks where we actually need them only that would be better.

crash-stats has a bunch of low volume crashes in various nsTreeBodyFrame functions

https://crash-stats.mozilla.com/search/?signature=~nsTreeBodyFrame&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Do you want to create a patch adding a null check for each signature?

Comment 18

[Wayne Mery (:wsmwk)]

Sorry, I'm not knowledgable to make a patch. Just trying to move the bug forward.

Looking at your query, many Thunderbird crash stacks are a mess.

Comment 19

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336

Comment 20

[Wayne Mery (:wsmwk)]

Let's ask m_kato about comment 17. recent crash for reference bp-bfc79cc9-7cb3-4dc5-a786-14e1b0180615 and bp-8984bd5f-7ef7-4736-ad11-85c3c0180520, which are still a mess, as first mentioned in comment 18. Perhaps add-on related?  https://crash-stats.mozilla.com/report/index/8984bd5f-7ef7-4736-ad11-85c3c0180520#tab-extensions has 76 add-ons! (seriously)


A different signature isn't quite such a mess nsTreeBodyFrame::GetSelectionRegion bp-f1672e22-2b76-4827-8b4c-e03a70180529

Comment 21

[Makoto Kato [:m_kato]]

(In reply to Wayne Mery (:wsmwk) from comment #20)
> Let's ask m_kato about comment 17. recent crash for reference
> bp-bfc79cc9-7cb3-4dc5-a786-14e1b0180615 and
> bp-8984bd5f-7ef7-4736-ad11-85c3c0180520, which are still a mess, as first
> mentioned in comment 18. Perhaps add-on related? 
> https://crash-stats.mozilla.com/report/index/8984bd5f-7ef7-4736-ad11-
> 85c3c0180520#tab-extensions has 76 add-ons! (seriously)

I guess that this is timing issue after frame (tree view) is destroyed.  But If addon uses XUL's tree view, it is related.  Maybe, Firefox doesn't use it now (and we are removing XUL code), so this is only on Thunderbird (and Seamonkey?).
 
> A different signature isn't quite such a mess
> nsTreeBodyFrame::GetSelectionRegion bp-f1672e22-2b76-4827-8b4c-e03a70180529

Same issue. mView is null.

Comment 22

[Wayne Mery (:wsmwk)]

(In reply to Makoto Kato [:m_kato] from comment #21)
> (In reply to Wayne Mery (:wsmwk) from comment #20)
> > ...
> > A different signature isn't quite such a mess
> > nsTreeBodyFrame::GetSelectionRegion bp-f1672e22-2b76-4827-8b4c-e03a70180529
> 
> Same issue. mView is null.

If you are willing to submit a patch that hits a couple locations [1] as mentioned in comment 17 would be great.  But if you are not (and I wouldn't hold it against you) then we should just close this, because I don't foresee anyone else doing it from being  virtually Thunderbird only, low crash rate and xul going away - well, some day.

[1] https://crash-stats.mozilla.com/search/?signature=~nsTreeBodyFrame&date=%3E%3D2018-04-08T19%3A22%3A18.000Z&date=%3C2018-07-08T19%3A22%3A18.000Z&_sort=-email&_sort=-user_comments&_sort=-date&_facets=signature&_facets=platform_version&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=user_comments#crash-reports

Comment 23

[Makoto Kato [:m_kato]]

Since this crash rate is low volume, I don't think this has to be fixed soon.  And we going away from XUL, so this is low priority now at least.

Comment 24

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.

Comment 25

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.

Comment 26

[Calixte Denizet (:calixte)]

There are still some crashes so reopen it.

Comment 27

[Wayne Mery (:wsmwk)]

Historically, and still today, the vast majority of crashes are Thunderbird. But most of those crashes are old versions [1].  In the past month there are only 5 crashes with current versions and one of those has bitdefender, which is known to cause issues.  With such a low crash rate, I suggest this bug is no longer interesting nor actionable.

[1] https://crash-stats.mozilla.com/signature/?product=Thunderbird&signature=nsTreeBodyFrame%3A%3APrefillPropertyArray&date=%3E%3D2018-10-07T08%3A19%3A58.000Z&date=%3C2019-01-07T07%3A19%3A58.000Z#graphs

Comment 28

[Wayne Mery (:wsmwk)]

Most of the old crashes referred to in comment 27 were probably version 52. The crash volume of version 68 is also much reduced compared to the prior version 60

Comment 29

[Wayne Mery (:wsmwk)]

Only a couple crashes per month for version 78, and even fewer user. Probably not actionable.