56570 - Cookie Manager crashes when cookie clicked with <Shift> key pressed down

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P3)

Description

[Loco]

BuildID:    2000-10-11-08 Mac, 2000-10-12-13 Win NT

Reproducible: Always

Steps to Reproduce:
1. Go to Preferences
2. Select Cookies
3. View Cookies you could delete
4. Select one cookie
5. With shift key, select another cookie
6. Click on <REMOVE COOKIE> button
7. Press <SHIFT> key
8. Click on another cookie.

CRASH.

Actual Results:  Crash
Expected Results:  Should just select the cookie.

This is a normal way to select items-I was doing nothing out of the ordinary by 
using the shift key.  This is common behavior, especially on the Mac.

Comment 1

[Loco]

Adding RTM, Crash KWs.  Hi Tom.:)

Comment 2

[Daniel Veditz [:dveditz]]

Too late for this... people will have to learn not to do that.

Comment 3

[Stephen P. Morse]

Dan, please reconsider your minusing of this one.  This is simply a case of 
dereferencing a null pointer with a very simple safe fix which I'll attach.  How 
can we not take such an ovious fix which is simply to add some defensive coding.

It has nothing to do with cookies but rather is a bug in the xul processing.  
However, since I already have the fix for it, I'll champion it through.

cc'ing bryner who added this code and hyatt who modified nearby code.


Index: nsXULTreeElement.cpp
===================================================================
RCS file: /cvsroot/mozilla/rdf/content/src/nsXULTreeElement.cpp,v
retrieving revision 1.54.2.1
diff -u -r1.54.2.1 nsXULTreeElement.cpp
--- nsXULTreeElement.cpp        2000/10/05 01:11:36     1.54.2.1
+++ nsXULTreeElement.cpp        2000/10/14 15:53:57
@@ -350,6 +350,8 @@

   while (PR_TRUE) {
       content = do_QueryInterface(currentItem);
+      if (!content)
+          break;
       content->GetTag(*getter_AddRefs(tag));
       if (tag && tag.get() == kTreeItemAtom)
           content->SetAttribute(kNameSpaceID_None, kSelectedAtom,

Comment 4

[Stephen P. Morse]

Attachment: Above patch repeated in an attachment

Comment 5

[Brian Ryner (not reading)]

r=bryner.  I can't say that I see offhand why this condition would come up, but
if we can avoid a crash at this point, we should go for it.

Comment 6

[Brendan Eich [:brendan]]

Why would that QI fail?

/be

Comment 7

[Daniel Veditz [:dveditz]]

If it crashes in widely used code like nsXULTreeElement we should fix it, since 
the cookie viewer may not be the only place this happens. Why does pressing the 
shift key make a difference?

The most likely cause for the QI to fail would be that currentItem is null. If 
that's true then it appears a lot of other assumptions in the code are wrong. 
I'd like a super-review from hyatt or waterson on this one.

Comment 8

[Brian Ryner (not reading)]

actually, this sounds like an old bug that's come back to bite us.  I'll debug
the real cause.

Comment 9

[Stephen P. Morse]

Yes, the mRawPtr in currentItem is null.

I don't know the logic here but I'm guessing that the story is something like 
this.  When you press the shift key and click on an item, that item is added to 
a list of selected items.  But since  you just deleted a list of selected items, 
you are now adding the item to a null list.  Well that's just a wild guess so if 
I'm not making any sense then just ignore this paragraph.

There's probably a more fundamental bug here and that should probably be looked 
into post rtm (on the trunk).  What I'm proposing for rtm (the branch) is this 
very safe piece of defensive coding that is known to stop the crash.

Here's the stack trace at the time that currentItem is null

nsXULTreeElement::SelectItemRange(nsXULTreeElement * const 0x034f0538, 
nsIDOMXULElement * 0x00000000, nsIDOMXULElement * 0x034110a4) line 354
XULTreeElementSelectItemRange(JSContext * 0x0341bae0, JSObject * 0x00e1c558, 
unsigned int 2, long * 0x00efbdfc, long * 0x0012ae10) line 554 + 33 bytes
js_Invoke(JSContext * 0x0341bae0, unsigned int 2, unsigned int 0) line 820 + 23 
bytes
js_Interpret(JSContext * 0x0341bae0, long * 0x0012b944) line 2620 + 15 bytes
js_Invoke(JSContext * 0x0341bae0, unsigned int 1, unsigned int 2) line 837 + 13 
bytes
js_InternalInvoke(JSContext * 0x0341bae0, JSObject * 0x00e1cf10, long 14800488, 
unsigned int 0, unsigned int 1, long * 0x0012badc, long * 0x0012ba6c) line 909 + 
20 bytes
JS_CallFunctionValue(JSContext * 0x0341bae0, JSObject * 0x00e1cf10, long 
14800488, unsigned int 1, long * 0x0012badc, long * 0x0012ba6c) line 3193 + 31 
bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x0341be70, void * 0x00e1cf10, 
void * 0x00e1d668, unsigned int 1, void * 0x0012badc, int * 0x0012bad8, int 0) 
line 907 + 33 bytes
nsJSEventListener::HandleEvent(nsIDOMEvent * 0x0345af14) line 154 + 64 bytes
nsXBLPrototypeHandler::ExecuteHandler(nsXBLPrototypeHandler * const 0x0349ae10, 
nsIDOMEventReceiver * 0x0348d1b8, nsIDOMEvent * 0x0345af14) line 314
DoMouse(nsIAtom * 0x024ed7a0, nsIXBLPrototypeHandler * 0x0349ae10, nsIDOMEvent * 
0x0345af14, nsIDOMEventReceiver * 0x0348d1b8) line 103
nsXBLMouseHandler::MouseClick(nsIDOMEvent * 0x0345af14) line 118 + 34 bytes
nsEventListenerManager::HandleEvent(nsIPresContext * 0x0347af00, nsEvent * 
0x0012d02c, nsIDOMEvent * * 0x0012cf48, nsIDOMEventTarget * 0x0348d1b8, unsigned 
int 2, nsEventStatus * 0x0012d34c) line 865 + 23 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x0348d1b0, nsIPresContext * 
0x0347af00, nsEvent * 0x0012d02c, nsIDOMEvent * * 0x0012cf48, unsigned int 2, 
nsEventStatus * 0x0012d34c) line 3301
nsXULElement::HandleDOMEvent(nsXULElement * const 0x033fb4d0, nsIPresContext * 
0x0347af00, nsEvent * 0x0012d02c, nsIDOMEvent * * 0x0012cf48, unsigned int 2, 
nsEventStatus * 0x0012d34c) line 3318 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x034071c0, nsIPresContext * 
0x0347af00, nsEvent * 0x0012d02c, nsIDOMEvent * * 0x0012cf48, unsigned int 2, 
nsEventStatus * 0x0012d34c) line 3318 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x03407100, nsIPresContext * 
0x0347af00, nsEvent * 0x0012d02c, nsIDOMEvent * * 0x0012cf48, unsigned int 1, 
nsEventStatus * 0x0012d34c) line 3318 + 39 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012d02c, nsIView * 0x00000000, 
unsigned int 1, nsEventStatus * 0x0012d34c) line 4472 + 47 bytes
PresShell::HandleEventWithTarget(PresShell * const 0x03478a10, nsEvent * 
0x0012d02c, nsIFrame * 0x00ed4758, nsIContent * 0x03407100, unsigned int 1, 
nsEventStatus * 0x0012d34c) line 4453 + 22 bytes
nsEventStateManager::CheckForAndDispatchClick(nsEventStateManager * const 
0x034ef090, nsIPresContext * 0x0347af00, nsMouseEvent * 0x0012d45c, 
nsEventStatus * 0x0012d34c) line 1861 + 61 bytes
nsEventStateManager::PostHandleEvent(nsEventStateManager * const 0x034ef098, 
nsIPresContext * 0x0347af00, nsEvent * 0x0012d45c, nsIFrame * 0x00ed4758, 
nsEventStatus * 0x0012d34c, nsIView * 0x0350aa30) line 935 + 28 bytes
PresShell::HandleEventInternal(nsEvent * 0x0012d45c, nsIView * 0x0350aa30, 
unsigned int 1, nsEventStatus * 0x0012d34c) line 4492 + 43 bytes
PresShell::HandleEvent(PresShell * const 0x03478a14, nsIView * 0x0350aa30, 
nsGUIEvent * 0x0012d45c, nsEventStatus * 0x0012d34c, int 1, int & 1) line 4407 + 
25 bytes
nsView::HandleEvent(nsView * const 0x0350aa30, nsGUIEvent * 0x0012d45c, unsigned 
int 28, nsEventStatus * 0x0012d34c, int 1, int & 1) line 379
nsViewManager2::DispatchEvent(nsViewManager2 * const 0x0347ab30, nsGUIEvent * 
0x0012d45c, nsEventStatus * 0x0012d34c) line 1439
HandleEvent(nsGUIEvent * 0x0012d45c) line 68
nsWindow::DispatchEvent(nsWindow * const 0x0350e1d4, nsGUIEvent * 0x0012d45c, 
nsEventStatus & nsEventStatus_eIgnore) line 682 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012d45c) line 703
nsWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 3895 + 
21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 
4105
nsWindow::ProcessMessage(unsigned int 514, unsigned int 4, long 589862, long * 
0x0012d7d8) line 2965 + 24 bytes
nsWindow::WindowProc(HWND__ * 0x000307dc, unsigned int 514, unsigned int 4, long 
589862) line 951 + 27 bytes
USER32! 

Comment 10

[Brian Ryner (not reading)]

I've tracked down the real cause of this.  The tree keeps a pointer to the
beginning of the selection, and it wasn't being cleared out when that item was
deleted.

Patch coming up, could everyone please give it a good testing?

Comment 11

[Brian Ryner (not reading)]

Attachment: patch (correctly null the pointer when the item is deleted)

Comment 12

[Stephen P. Morse]

I get the following link-time error when I apply the latest patch:

   Creating library .\WIN32_D.OBJ\rdf.lib
         and object .\WIN32_D.OBJ\rdf.exp rdfcontent_s.lib(nsXULTreeElement.obj)
   : error LNK2001: unresolved external symbol 
      "public: virtual unsigned int __stdcall 
         nsIXULTreeContent::CheckSelection(class nsIDOMXULElement *)" 
   (?CheckSelection@nsIXULTreeContent@@UAGIPAVnsIDOMXULElement@@@Z)

Comment 13

[Stephen P. Morse]

I believe that the added line to nsIXULTreeContent.h should be:

+    NS_IMETHOD CheckSelection(nsIDOMXULElement* aDeletedItem) = 0;

instead of:

+    NS_IMETHOD CheckSelection(nsIDOMXULElement* aDeletedItem);

Comment 14

[Stephen P. Morse]

bryner's patch (with correction indicated above) indeed fixes the problem.  No 
more crash when selecting cookies using the shift key.

My recommendation would be to go with my simple, safe, risk-free, two-line patch 
on the branch and put bryner's patch on the trunk.  Bryner, now that you 
understand the underlying problem, can you tell if there is any scenero that 
will still lead to a crash with just my patch applied?

Comment 15

[Brian Ryner (not reading)]

You're right, =0 should be on the end.  I forgot to re-do the patch after fixing
that.

There is still a potential for crash with your original patch.  The reason is
that currentItem could in fact point to deleted memory (but be a non-null
pointer).  So, there's a decent chance that the QueryInterface call could crash.

Comment 16

[David Hyatt]

Bryner's fix is the only one that I will accept as tree widget module owner on
either the branch or the trunk.

The null pointer fix isn't the full fix, and bryner's fix is just as safe (and
heads off other problems/crashes).

a=hyatt on bryner's patch.

Comment 17

[Brendan Eich [:brendan]]

r=[e-mail] for the trunk.

(I wish you tree guys would stop mixing two space indentation into waterson's
four-space-indented nsXULElement.cpp code, but you were modifying a paragraph
where the deed was already done.)

/be

Comment 18

[Stephen P. Morse]

OK, I've taken this as far as I can.  Reassigning to bryner.

Comment 19

[Brian Ryner (not reading)]

so, what should I do with this now?  remove [rtm need info]?

Comment 20

[Brendan Eich [:brendan]]

You're supposed to restore the [rtm+] once info (r= and a=/sr= at least) has
been provided.

/be

Comment 21

[Brian Ryner (not reading)]

ok, but not in the URL field...

Comment 22

[Brendan Eich [:brendan]]

That'll learn ya! ;-)

/be

Comment 23

[Loco]

Changing QA Contact per managerial request.

Comment 24

[Peter Trudelle]

PDT says rtm++, okay to land on trunk and branch.  John, please pound on this

Comment 25

[Brian Ryner (not reading)]

checked in on branch and trunk.

yes, everyone please pound on this.

Comment 26

[John Morrison]

Ok, with the evening verif moz builds (on the trunk), the crash for the cookie 
dialog is gone, and I also tried similar actions and variations in the bookmark 
manager and mailnews threadPane, without crashing and without any odd 
behaviours. Looks good to me.

Comment 27

[Loco]

VERIFIED on Mac 10-18-08, NT 10-18-08, Linux 10-18-09.

Comment 28

[Loco]

Ack, sorry about the spam.  I'm supposed to only add the vtrunk keyword and leave 
it as resolved so that we still verify it later.  My bad.

Comment 29

[Loco]

Yes, RESOLVED FIXED, now with vtrunk in KW.

Comment 30

[Asa Dotzler [:asa]]

Verified Fixed on Mozilla trunk builds.
linux 101908 RedHat 6.2
win32 101904 NT 4
mac 101904 Mac OS9
Setting bug to Verified and removing vtrunk keyword