crash in nsXULTreeAccessible::InvalidateCache when deleting cookie

RESOLVED FIXED in mozilla16

Status

()

Core
Disability Access APIs
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Susanne Jaeger, Assigned: capella)

Tracking

({crash})

Trunk
mozilla16
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120516 Firefox/14.0a2 SeaMonkey/2.11a2
Build ID: 20120516013004

Steps to reproduce:

select existing cookie in Data Manager and hit remove


Actual results:

Seamonkey crashs 
one of a series of crash reports <https://crash-stats.mozilla.com/report/index/bp-7700bb06-91a2-42d7-8791-d16d32120521>


Expected results:

cookie should be removed
Crash Signature: [@ nsXULTreeAccessible::InvalidateCache ]
Component: General → Disability Access APIs
Product: SeaMonkey → Core
QA Contact: general → accessibility-apis
looks like the case where all rows go away then the tree view isn't handled, I cna try and write a patch if nobody gets to it before I'm done reviews.

Updated

5 years ago
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsXULTreeAccessible::InvalidateCache ] → [@ nsXULTreeAccessible::InvalidateCache ] [@ nsXULTreeAccessible::InvalidateCache(int, int) ]
Ever confirmed: true
Keywords: crash
OS: Linux → All
Hardware: x86_64 → All

Updated

5 years ago
Summary: crash when deleting cookie → crash in nsXULTreeAccessible::InvalidateCache when deleting cookie

Comment 2

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> looks like the case where all rows go away then the tree view isn't handled,
> I cna try and write a patch if nobody gets to it before I'm done reviews.

Trevor, any luck on this?
(In reply to alexander :surkov from comment #2)
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> > looks like the case where all rows go away then the tree view isn't handled,
> > I cna try and write a patch if nobody gets to it before I'm done reviews.
> 
> Trevor, any luck on this?

fell off my radar :(
(Assignee)

Comment 4

5 years ago
Created attachment 632096 [details] [diff] [review]
Patch (v1)

Crash bug...
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attachment #632096 - Flags: review?(surkov.alexander)
Comment on attachment 632096 [details] [diff] [review]
Patch (v1)

>+  if (!mTreeView)
>+    return;

I think the real fix here is to stop using DOM events, and instead have layout call into a11y for this, but this seems fine for now.  However I tend to think we should start by checking mTreeView, and if its null just flushing the entire cache, since a tree with no view definitionally has no rows.
(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> Comment on attachment 632096 [details] [diff] [review]
> Patch (v1)
> 
> >+  if (!mTreeView)
> >+    return;
> 
> I think the real fix here is to stop using DOM events, and instead have
> layout call into a11y for this, but this seems fine for now.  However I tend
> to think we should start by checking mTreeView, and if its null just
> flushing the entire cache, since a tree with no view definitionally has no
> rows.

Checking mTreeView near the top and flushing the cache if null makes some sense and missing firing a few hide events seems less worrisome if we already have no mTreeView.

Are there STR for this crash in FF?
(Assignee)

Comment 7

5 years ago
Created attachment 632975 [details] [diff] [review]
Patch (v2)

So, you suggest tweaking it this way?
Attachment #632096 - Attachment is obsolete: true
Attachment #632096 - Flags: review?(surkov.alexander)
(Assignee)

Comment 8

5 years ago
Comment on attachment 632975 [details] [diff] [review]
Patch (v2)

fyi - switching review request to tbsaunde since surkov is on vacation ... I didn't combine the IsDefunct() check with the !mTreeView check, that could be done also if you suggest
Attachment #632975 - Flags: review?(trev.saunders)
Attachment #632975 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 9

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=77f704ba95f8

Comment 10

5 years ago
(In reply to David Bolter [:davidb] from comment #6)
> Checking mTreeView near the top and flushing the cache if null makes some
> sense

what sense does the cache flushing make? it should be flushed at this point. I'd say assert if there's cached items and no tree view.
(Assignee)

Comment 11

5 years ago
Per quick IRC chat with surkov, this doesn't cause harm, is just confusing to him.
(Assignee)

Comment 12

5 years ago
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=eb67e4c9f7df
Target Milestone: --- → mozilla16
(In reply to alexander :surkov from comment #10)
> (In reply to David Bolter [:davidb] from comment #6)
> > Checking mTreeView near the top and flushing the cache if null makes some
> > sense
> 
> what sense does the cache flushing make? it should be flushed at this point.
> I'd say assert if there's cached items and no tree view.

remember we handle the tree view going away sync, but do the row update based on DOM event, so I think its possible the DOM event shows up after the tree view has gone away.
https://hg.mozilla.org/mozilla-central/rev/8aa06f0cc714
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 15

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #13)

> > what sense does the cache flushing make? it should be flushed at this point.
> > I'd say assert if there's cached items and no tree view.
> 
> remember we handle the tree view going away sync, but do the row update
> based on DOM event, so I think its possible the DOM event shows up after the
> tree view has gone away.

exactly but also that means cache is empty (we do this when tree view is nulled out what happens before those DOM events). Correct?
(In reply to alexander :surkov from comment #15)
> (In reply to Trevor Saunders (:tbsaunde) from comment #13)
> 
> > > what sense does the cache flushing make? it should be flushed at this point.
> > > I'd say assert if there's cached items and no tree view.
> > 
> > remember we handle the tree view going away sync, but do the row update
> > based on DOM event, so I think its possible the DOM event shows up after the
> > tree view has gone away.
> 
> exactly but also that means cache is empty (we do this when tree view is
> nulled out what happens before those DOM events). Correct?

yeah, somehow I was sure we didn't clear the cache on the treeview changing.
You need to log in before you can comment on or make changes to this bug.