Last Comment Bug 757203 - crash in nsXULTreeAccessible::InvalidateCache when deleting cookie
: crash in nsXULTreeAccessible::InvalidateCache when deleting cookie
Status: RESOLVED FIXED
: crash
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla16
Assigned To: Mark Capella [:capella]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-21 14:07 PDT by Susanne Jaeger
Modified: 2012-06-18 03:49 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (1.00 KB, patch)
2012-06-11 18:10 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v2) (937 bytes, patch)
2012-06-13 17:38 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Susanne Jaeger 2012-05-21 14:07:14 PDT
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) 2012-05-21 20:14:19 PDT
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 2012-06-01 10:30:47 PDT
(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) 2012-06-01 10:43:01 PDT
(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] 2012-06-11 18:10:33 PDT
Created attachment 632096 [details] [diff] [review]
Patch (v1)

Crash bug...
Comment 5 Trevor Saunders (:tbsaunde) 2012-06-11 19:41:40 PDT
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] 2012-06-13 13:51:44 PDT
(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] 2012-06-13 17:38:02 PDT
Created attachment 632975 [details] [diff] [review]
Patch (v2)

So, you suggest tweaking it this way?
Comment 8 Mark Capella [:capella] 2012-06-13 17:49:03 PDT
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] 2012-06-14 19:19:47 PDT
https://tbpl.mozilla.org/?tree=Try&rev=77f704ba95f8
Comment 10 alexander :surkov 2012-06-15 00:41:02 PDT
(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] 2012-06-15 01:10:01 PDT
Per quick IRC chat with surkov, this doesn't cause harm, is just confusing to him.
Comment 12 Mark Capella [:capella] 2012-06-15 01:42:14 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=eb67e4c9f7df
Comment 13 Trevor Saunders (:tbsaunde) 2012-06-15 03:18:07 PDT
(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] 2012-06-16 06:53:32 PDT
https://hg.mozilla.org/mozilla-central/rev/8aa06f0cc714
Comment 15 alexander :surkov 2012-06-16 22:15:24 PDT
(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) 2012-06-18 03:49:45 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.