757203 - crash in nsXULTreeAccessible::InvalidateCache when deleting cookie

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Susanne Jaeger]

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

Comment 1

[Trevor Saunders (:tbsaunde)]

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.

Comment 2

[alexander :surkov (:asurkov)]

(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?

Comment 3

[Trevor Saunders (:tbsaunde)]

(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 :(

Comment 4

[Mark Capella [:capella]]

Attachment: Patch (v1)

Crash bug...

Comment 5

[Trevor Saunders (:tbsaunde)]

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.

Comment 6

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

(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?

Comment 7

[Mark Capella [:capella]]

Attachment: Patch (v2)

So, you suggest tweaking it this way?

Comment 8

[Mark Capella [:capella]]

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

Comment 9

[Mark Capella [:capella]]

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

Comment 10

[alexander :surkov (:asurkov)]

(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.

Comment 11

[Mark Capella [:capella]]

Per quick IRC chat with surkov, this doesn't cause harm, is just confusing to him.

Comment 12

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=eb67e4c9f7df

Comment 13

[Trevor Saunders (:tbsaunde)]

(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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8aa06f0cc714

Comment 15

[alexander :surkov (:asurkov)]

(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?

Comment 16

[Trevor Saunders (:tbsaunde)]

(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.