116413 - negative VoidArray index in GetMenuAt() in View menu on Mac

Status: RESOLVED FIXED

(Core :: XUL, defect, P2)

Description

[Randell Jesup [:jesup] (needinfo me)]

From bug 96108:

------- Additional Comments From [e-mail]  2001-12-21 08:56 -------
I hit the assertion "###!!! ASSERTION: nsVoidArray::ElementAt(negative index) -
note on bug 96108: 'aIndex >= 0', file nsVoidArry.h, line 77", coming from
http://lxr.mozilla.org/seamonkey/source/widget/src/mac/nsDynamicMDEF.cpp#465.
gPreviousMenuHandleStack.Count() is 0. I tried to open the View menu on my mac
build.
-------

Patch to be attached.  I might be able to patch it with less perf hit (not that
it will be noticable) if i had the stack backtrace.

Can cause crash in opt build.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: patch ready for review

This will fix the problem.  I'd prefer to fix the caller; I looked at several
and didn't see how we'd get in with an empty list.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Looking for review of a trivial fix for a VoidArray negative-index use missed by
the patch.

Comment 3

[Johnny Stenback (:jst)]

Comment on attachment 62531 [details] [diff] [review]
patch ready for review

sr=jst.

Go ahead and check this one in if you want, no need for a separate r= for
something this trivial.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Fix checked in, thanks

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

THis was nsSupportsArray, not nsVoidArray.  I HATE that Supports and Void are so
close but don't inherit or share; it bites us again and again.  We'll need to
track down the caller's stack and make them not do that.  

Comment 6

[Peter Van der Beken [:peterv]]

 Calling chain using A6/R1 links
  Back chain  ISA  Caller
  0A50BDBD    PPC  FFCEC3FC  EmToNatEndMoveParams+00014
  0A50BD40    PPC  009D96E0  GetSubmenuParentChoosable+00178
  0A50BCF0    PPC  009D9ED0  GetSubmenuParentChoosable+00968
  0A50BC50    PPC  009D9B8C  GetSubmenuParentChoosable+00624
  0A50BBB0    PPC  009DA2B4  GetSubmenuParentChoosable+00D4C
  0A50BB70    PPC  009DA7FC  GetSubmenuParentChoosable+01294
  0A50BB20    PPC  009D2DD8  CallMenuBar+00138
  0A50BA1A    68K  01D4D5C8  
  0A50B9CB    PPC  FFCEC3FC  EmToNatEndMoveParams+00014
  0A50B980    PPC  01A7F088  'MBDF 003F 0002'+00148
  0A50B940    PPC  009DEE00  StandardMBDF+00200
  0A50B860    PPC  009E1AC0  StandardMBDF+02EC0
  0A50B810    PPC  FFD0B760  CalcMenuSize+00018
  0A50B6F3    PPC  FFCEC3FC  EmToNatEndMoveParams+00014
  0A50B680    PPC  0823900C  nsDynamicMDEFMain+003EC
  0A50B590    PPC  08239374  nsDynamicSizeTheMenu(MenuInfo**)+00014
  0A50B550    PPC  082395C8  nsDoMagic(MenuInfo**)+00118
  0A50B500    PPC  082399B4  nsBuildMenu(MenuInfo**, int)+00394
  0A50B360    PPC  08239F3C  nsPostBuild(nsIMenu*, MenuInfo**, int)+0006C
  0A50B300    PPC  086AB71C  nsDebug::Assertion(const char*, const char*, const
char*, int)+0005C

Randell, let me know if you need anything else. This assertion is getting rather
annoying.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

I'll try to solve this Monday

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Patch to stop 3 negative array refs

Please review/test.  I went over the file with a fine-tooth comb; I believe I
caught all possible negative index references.

Comment 9

[Peter Van der Beken [:peterv]]

Comment on attachment 63814 [details] [diff] [review]
Patch to stop 3 negative array refs

>-  if(isChild || (gPreviousMenuHandleStack[gPreviousMenuHandleStack.Count() - 1] != theMenu))
>+  if(isChild ||
>+     (gPreviousMenuHandleStack.Count() > 0 &&
>+      gPreviousMenuHandleStack[gPreviousMenuHandleStack.Count() - 1] != theMenu))

This breaks hierarchical menus. I came up with this which seems to work:

+  PRInt32 count = gPreviousMenuHandleStack.Count();
+  if(isChild ||
+     ((count > 0 &&
+	gPreviousMenuHandleStack[gPreviousMenuHandleStack.Count() - 1] !=
theMenu) ||
+      (count <= 0 && theMenu)))

Not really sure if this is the best way to do it, but you need to handle the
case
where theMenu is non-null and count <= 0.

>-  *aMenu = (MenuHandle) gPreviousMenuHandleStack[gPreviousMenuHandleStack.Count() - 1];
>-  gPreviousMenuHandleStack.RemoveElementAt(gPreviousMenuHandleStack.Count() - 1);
>+  if (gPreviousMenuHandleStack.Count() > 0)
>+  {
>+    *aMenu = (MenuHandle) gPreviousMenuHandleStack[gPreviousMenuHandleStack.Count() - 1];
>+    gPreviousMenuHandleStack.RemoveElementAt(gPreviousMenuHandleStack.Count() - 1);
>+  }

Not sure if we should set *aMenu to null here, when .Count() <= 0.

Comment 10

[Peter Van der Beken [:peterv]]

Sfraser checked in fixes for two of the three spots you found.
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/widget/src/mac&command=DIFF_FRAMESET&file=nsDynamicMDEF.cpp&rev1=1.21&rev2=1.22&root=/cvsroot

Leaves the on in nsPopMenuHandle, not sure if we need to do anything there.

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

I'm pretty sure the nsPopMenuHandle fix is needed.  Updating my tree and I'll
post a new patch ASAP to get this in.  (I've just finished getting my system
back together after a disk drive started squeaking instead of reading data -
lost my mozilla development tree and all patches I hadn't posted, along with
lots of other things).

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

The nsPopMenuHandle fix is needed, but there are no uses of nsPopMenuHandle, so
it's non-critical.  (We nsPushMenuHandle and nsPushMenu, but we don't pop menus
or menu handles ever.)

Changing target.  We should probably remove the dead code unless it's there for
a reason - some future use?  Adding previous modifiers of this code for comment.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

The code in question (nsPopMenuHandle fix) is dead code at this point; nothing
calls it.  We should either check in the fix for it, or nuke the function if it
doesn't serve a purpose.

Retargetting for 1.0 as a cleanup/footprint issue.  Very minor, could be ignored
though I dislike leaving known crasher code in even if it can't get called (now).

Comment 14

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 15

[timeless]

Attachment: last bit

i know we don't believe this code is alive, but i'd like to close this bug.

Comment 16

[Simon Paquet [:sipaq]]

This bug is targeted at a Mac classic platform/OS, which is no longer supported
by mozilla.org. Please re-target it to another platform/OS if this bug applies
there as well or resolve this bug.

I will resolve this bug as WONTFIX in four weeks if no action has been taken.
To filter this and similar messages out, please filter for "mac_cla_reorg".

Comment 17

[Joe Francis]

Comment on attachment 112055 [details] [diff] [review]
last bit

r=mozeditor

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 112055 [details] [diff] [review]
last bit

sr=jst

Comment 19

[timeless]

last bit checked in