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

RESOLVED FIXED in mozilla1.0.1

Status

()

P2
critical
RESOLVED FIXED
17 years ago
15 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

({crash})

Trunk
mozilla1.0.1
PowerPC
Mac System 9.x
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

17 years ago
From bug 96108:

------- Additional Comments From peterv@netscape.com  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.
(Assignee)

Updated

17 years ago
Depends on: 96108
Keywords: crash
(Assignee)

Comment 1

17 years ago
Created attachment 62531 [details] [diff] [review]
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.
(Assignee)

Comment 2

17 years ago
Looking for review of a trivial fix for a VoidArray negative-index use missed by
the patch.
Status: NEW → ASSIGNED
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.
Attachment #62531 - Flags: superreview+
Attachment #62531 - Flags: review+
(Assignee)

Comment 4

17 years ago
Fix checked in, thanks
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 5

17 years ago
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.  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
 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.
(Assignee)

Comment 7

17 years ago
I'll try to solve this Monday
Status: REOPENED → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.8
(Assignee)

Comment 8

17 years ago
Created attachment 63814 [details] [diff] [review]
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.
Attachment #62531 - Attachment is obsolete: true
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.
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.
(Assignee)

Comment 11

17 years ago
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).
(Assignee)

Comment 12

17 years ago
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.
Severity: major → normal
Target Milestone: mozilla0.9.8 → mozilla0.9.9
(Assignee)

Comment 13

17 years ago
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).
Target Milestone: mozilla0.9.9 → mozilla1.0
(Assignee)

Updated

17 years ago
Severity: normal → trivial
Keywords: mozilla1.1
Target Milestone: mozilla1.0 → mozilla1.0.1

Updated

16 years ago
Hardware: PC → Macintosh

Comment 14

16 years ago
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.
Severity: trivial → critical

Comment 15

16 years ago
Created attachment 112055 [details] [diff] [review]
last bit

i know we don't believe this code is alive, but i'd like to close this bug.
Attachment #63814 - Attachment is obsolete: true

Updated

15 years ago
Attachment #112055 - Flags: review?(mozeditor)
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

15 years ago
Comment on attachment 112055 [details] [diff] [review]
last bit

r=mozeditor
Attachment #112055 - Flags: review?(mozeditor) → review+

Updated

15 years ago
Attachment #112055 - Flags: superreview?(jst)
Comment on attachment 112055 [details] [diff] [review]
last bit

sr=jst
Attachment #112055 - Flags: superreview?(jst) → superreview+

Comment 19

15 years ago
last bit checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.