382194 - ASSERTION: nsMenuX::AttributeChanged: WRITE SHOW CODE FOR SUBMENU

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jesse Ruderman]

I see this assertion a lot.  It's annoying, in part because it's all-caps.

###!!! ASSERTION: nsMenuX::AttributeChanged: WRITE SHOW CODE FOR SUBMENU.: 'PR_FALSE', file /Users/jruderman/trunk/mozilla/widget/src/cocoa/nsMenuX.mm, line 1123

Comment 1

[Jesse Ruderman]

For example, I see this when I load http://www.regarde.org/blog/.

Comment 2

[Jesse Ruderman]

Attachment: testcase

The testcase involves two RSS files.  When I reload it a bunch of times quickly, I see the assertion about half of the time.

Comment 3

[Colin Barrett [:cbarrett]]

Attachment: reduced XUL testcase

These assertions do in fact reveal a bug. See attached XUL, which you need to run with -chrome file:///path/to/testcase.xul. There is a submenu that is initially hidden.

Clicking Show Menu and Hide Menu show and hide the menu properly on Linux. On Mac OS X, assertions fire and the menu does not show up (and thus can't rehide).

I'm not sure what the best approach is. There are probably a lot of things wrong with the menu code that are masked by the fact that we rebuild so much. , For starters, when items are marked invisible, they aren't moved out of mMenuItemsArray and into the mHiddenMenuItemsArray array.

I think having one array with all menu items, visible and invisible, and updating GetItemCount and GetItemAt to filter out invisible ones will be part of the solution?

Comment 4

[Adam Guthrie]

This is blocking enabling fatal assertions on the new OS X leak tinderbox.

Comment 5

[Jesse Ruderman]

Adam, we should turn this into a warning for now if that's the case.  Having assertions fatal on tinderbox is important for catching regressions.  If we wait until this bug is fixed to make assertions fatal we'll probably find that other regressions crept in and we'll have to wait for those too.

Comment 6

[Adam Guthrie]

Attachment: change ASSERTION into a WARNING

Comment 7

[Colin Barrett [:cbarrett]]

Comment on attachment 273850 [details] [diff] [review]
change ASSERTION into a WARNING

I can't r+ this, as I'm not an owner or peer, and you're not an owner or peer asking for review.

I approve of making this into a warning though, since there is a bug and it's actively being worked on.

Comment 8

[Colin Barrett [:cbarrett]]

(In reply to comment #3)
> I think having one array with all menu items, visible and invisible, and
> updating GetItemCount and GetItemAt to filter out invisible ones will be part
> of the solution?

Alright, I've implemented this, and it sucks. It sucks a lot.

For one thing, it makes GetItemAt not O(1). For another, it means there are now two ways to iterate over the set of items -- if you use GetItemAt you don't get invisible items, but it's slower. If you access mMenuItemsArray directly, you get all of them. This is pretty confusing and makes for some messy code.

One very interesting thing is that as far as I can tell, nsIMenu and nsIMenuItem aren't used anywhere except internally in nsIMenuBar. In particular, nsIMenu's GetItemAt and GetItemCount aren't used anywhere except in nsMenuX.mm.

With this in mind I think it might just be best to change the comment in the header that GetItemAt only includes visible items. There's only one place I found (so far) that relies on this behavior, and it's in nsMenuX, so adding in code to filter out invisible items won't be that hard (it's about 5 extra lines).

Josh (or anyone cc'd to this bug), I'd appreciate any input. In particular, if I'm incorrect about nsIMenu not being used outside of widgetland (there doesn't even seem to be an idl?), I'd love to know.

Comment 9

[Colin Barrett [:cbarrett]]

Attachment: fix

Here's my proposed fix. The diff is a little hard to read, unfortunately.

Basically, mMenuItemsArray stores all menu items, visible or hidden. I added some methods to nsIMenu. CountVisibleBefore supports the case when the parent is not an nsIMenuBar. AttributeChanged correctly removes and adds submenus, fixing this bug. It also uses NSMenu's API smarter in one case.

Comment 10

[Colin Barrett [:cbarrett]]

Comment on attachment 277008 [details] [diff] [review]
fix

sorry about that sr request, roc.

Comment 11

[Josh Aas]

I tried your XUL test case, and with your patch applied the menu *item* "1" shows up when I hit the show button but but it has no submenu. Is that intentional? Why can't I mouse over it and see items "a" and "b" in the submenu?

       } // if not visible
+      else
+        NS_WARNING("You're showing the menu twice, please stop");

Please just remove the else/warning here and in the other place too (hiding). There is no need for it. Note that the comments after the braces on the lines above the else are unnecessary too ("if not visible").

+    PRUint32                    mVisibleItemsCount;

To make sure I understand correctly, your new variable mVisibleItemsCount is essentially just caching? That could be derived at any time by going through the menu items array, which would be slow. If it is indeed just caching, please make a note of that where it is declared in the header.

+// XXXcbarrett for people who want to filter, should there

This part of the comment should be removed, looks like you just forgot to finish it.

Looks good so far, but I'm not done reviewing.

Comment 12

[Colin Barrett [:cbarrett]]

(In reply to comment #11)
> I tried your XUL test case, and with your patch applied the menu *item* "1"
> shows up when I hit the show button but but it has no submenu. Is that
> intentional? Why can't I mouse over it and see items "a" and "b" in the
> submenu?

No, that's not intentional. I must have accidentally deleted that line or something.

>        } // if not visible
> +      else
> +        NS_WARNING("You're showing the menu twice, please stop");
> 
> Please just remove the else/warning here and in the other place too (hiding).
> There is no need for it. Note that the comments after the braces on the lines
> above the else are unnecessary too ("if not visible").

Done.

> 
> +    PRUint32                    mVisibleItemsCount;
> 
> To make sure I understand correctly, your new variable mVisibleItemsCount is
> essentially just caching? That could be derived at any time by going through
> the menu items array, which would be slow. If it is indeed just caching, please
> make a note of that where it is declared in the header.

Done

> +// XXXcbarrett for people who want to filter, should there
> 
> This part of the comment should be removed, looks like you just forgot to
> finish it.

Yeah, I did. Done.

> Looks good so far, but I'm not done reviewing.

Great! I'll post the new patch in a sec.

Comment 13

[Colin Barrett [:cbarrett]]

Attachment: fix addressing 1st round of review comments

Here's the fix addressing the above review comments.

Comment 14

[Josh Aas]

I'm wondering if the test case is broken or your patch is broken in that it doesn't show the actual submenu when the hidden attribute is removed. Are you saying you know for a fact that it is the test case that is broken?

Comment 15

[Colin Barrett [:cbarrett]]

The patch was broken -- I fixed it.

I accidentally deleted a call to setSubmenu :)

Comment 16

[Josh Aas]

Comment on attachment 277260 [details] [diff] [review]
fix addressing 1st round of review comments

Your algorithm for nsMenuX::GetVisibleItemAt looks incorrect to me. Say you have a menu items array like this:

0. a (hidden)
1. b (hidden)
2. c (visible)
3. d (visible)

A call GetVisibleItemAt(1) will start searching that array at position 1 and return c when it should return d, the second visible item in the array.

Comment 17

[Colin Barrett [:cbarrett]]

Attachment: fix addressing 2nd round of review comments.

Fixed algorithmic problem and addressed review concerns from IRC.

Comment 18

[Colin Barrett [:cbarrett]]

Attachment: fix, this time using operators correctly.

Argh. I don't know what I was thinking. Thou shalt not code on sunday night, apparently.

Comment 19

[Josh Aas]

Comment on attachment 277367 [details] [diff] [review]
fix, this time using operators correctly.

+  // Traverse the arry until we find the the item we're looking for.

"array"

+  PRUint32 i, visibleNodeIndex = 0;
+  for (i = 0; i < count; i++) {

Declare "i" in the for loop (PRUin32 i = 0).

+    // If we find a visible node, see if we're at the right index
+    if (itemContent && !NodeIsHiddenOrCollapsed(itemContent)) {      
+      if (++visibleNodeIndex == aPos)
+        break;
+    }

This means your algorithm is still incorrect. You're incrementing visibleNodeIndex *before* the comparison to aPos - that can't possibly work if you're searching for an item at index 0. You need to increment after the "if" and its break.

On that note, I always prefer to use "foo++" for consistency, the difference between ++foo and foo++ makes things hard to read. Also, I'd avoid ever doing either of those within a test unless it is strictly necessary. I like nice explicit increments or decrements on their own line. Please change this stuff in your patch so it matches most of the rest of cocoa widgets.

Comment 20

[Jesse Ruderman]

I prefer ++foo to foo++ because I think pre-increment is a "more sane operator", but I agree that increments should usually be separate statements.

Comment 21

[Colin Barrett [:cbarrett]]

Attachment: fix

Sorry about all the revisions -- my fault for not calling it quits when I should have.

Josh, you mentioned on IRC about why I made the changes to CountVisibleBefore that I did? I extended CountVisibleBefore to handle the case when mParent wasn't an nsIMenuBar. This happens in nsMenuX::AttributeChanged. I also removed the gotThisMenu variable, and instead returned early. I can add it back if you don't like early returns.

Comment 22

[Colin Barrett [:cbarrett]]

There's a typo in the patch I just put up (is this bug cursed?). PRUin32 should be PRUint32. I forgot to regenerate the patch before attaching it. Sorry!

Comment 23

[Colin Barrett [:cbarrett]]

Attachment: fix

Here's the patch with the typo corrected

Comment 24

[Josh Aas]

Comment on attachment 277444 [details] [diff] [review]
fix

The algorithm is still wrong.

+    // If we find a visible node, see if we're at the right index
+    if (itemContent && !NodeIsHiddenOrCollapsed(itemContent)) {
+      visibleNodeIndex++;
+      if (visibleNodeIndex == aPos)
+        break;
+    }

This is no different at all than what you were doing before, the prefix ++. You need to increment *after* the test.

It worried me that you're not seeing this as bugs in the menu impl during your testing. As far as I can tell this algorithm will fail every single time no matter what the conditions are.

Comment 25

[Colin Barrett [:cbarrett]]

Attachment: proposed fix

Here's a greatly revised patch. I went through and looked at what actually was getting called and where. I've removed some of the use of GetVisibleItemAt, and added a new helper method (MenuNodeIsVisible) to avoid code duplication.

I also added an optimization to GetVisibleItemAt -- if there are no invisible items, we can provide direct access. The menu icon code was hitting GetVisibleItemAt, and for a huge list of bookmarks, all with favicons, inserting them all turned out to be O(n^2) with GetVisibleItemAt. Thankfully, it looks like those menus don't have any hidden items.

Comment 26

[Josh Aas]

Comment on attachment 277469 [details] [diff] [review]
proposed fix

+    // Find the content for this item in the menu, be it a MenuItem or a Menu
+    nsCOMPtr<nsIContent> itemContent;
+    nsCOMPtr<nsIMenuItem> menuItem = do_QueryInterface(item);
+    if (menuItem)
+      menuItem->GetMenuItemContent(getter_AddRefs(itemContent));
+    else {
+      nsCOMPtr<nsIMenu> menu = do_QueryInterface(item);
+      if (menu)
+        menu->GetMenuContent(getter_AddRefs(itemContent));
+    }
+
+    // If we find a visible node, see if we're at the right index
+    if (itemContent && !NodeIsHiddenOrCollapsed(itemContent)) {
+      if (aPos == visibleNodeIndex) {
+        aMenuItem = item;
+        NS_IF_ADDREF(aMenuItem);
+        return NS_OK;
+      }
+      visibleNodeIndex++;
+    }
+  }

Here in nsMenuX::GetVisibleItemAt, why aren't you using your new helper method MenuNodeIsVisible?

+  nsCOMPtr<nsIMenu> menuParent = do_QueryInterface(mParent);

What is the point of doing this at the top of  nsMenuX::AttributeChanged when it is only used in the hidden/collapsed case? Please move it into that block so we don't always do it.

-    nsISupports* aTargetMenuItem;
-    targetMenu->GetItemAt((PRUint32)aPos, aTargetMenuItem);
+    nsCOMPtr<nsISupports> aTargetMenuItem;
+    targetMenu->GetVisibleItemAt((PRUint32)aPos, *getter_AddRefs(aTargetMenuItem));

Why this change in MyMenuEventHandler? I'm not saying it is wrong, I'm just asking why you made it.

Comment 27

[Josh Aas]

Attachment: fix v2.0

There were some changes I wanted to make that I didn't want to have to write out in the bug, so I made them in this revision of Colin's patch and addressed my last review comments. I didn't make any major changes, this is still almost entirely Colin's code.

I'm just labeling this revision as 2.0 so we can start using a numbering scheme - referring to particular revisions is hard otherwise.

Comment 28

[Josh Aas]

Attachment: josh's changes to Colin's patch

These are the changes I made to Colin's patch so it is easier to see what I did. This is a diff of nsMenuX.mm with Colin's patch vs. with my patch.

Colin, if you can just review my changes to your patch then once you give r+ I'll r+ fix v2.0 as your patch. A little confusing but it'll work...

Comment 29

[Colin Barrett [:cbarrett]]

(In reply to comment #26)
> (From update of attachment 277469 [details] [diff] [review])
> -    nsISupports* aTargetMenuItem;
> -    targetMenu->GetItemAt((PRUint32)aPos, aTargetMenuItem);
> +    nsCOMPtr<nsISupports> aTargetMenuItem;
> +    targetMenu->GetVisibleItemAt((PRUint32)aPos,
> *getter_AddRefs(aTargetMenuItem));
> 
> Why this change in MyMenuEventHandler? I'm not saying it is wrong, I'm just
> asking why you made it.

The other way was causing a crash. I'm not sure why, XPCOM reference counting is still a bit foreign to me. using an nsCOMPtr worked every where else, so I changed it.

r=me on your changes.

Comment 30

[Colin Barrett [:cbarrett]]

Checking in widget/src/cocoa/nsMenuX.h;
/cvsroot/mozilla/widget/src/cocoa/nsMenuX.h,v  <--  nsMenuX.h
new revision: 1.24; previous revision: 1.23
done
Checking in widget/src/cocoa/nsMenuX.mm;
/cvsroot/mozilla/widget/src/cocoa/nsMenuX.mm,v  <--  nsMenuX.mm
new revision: 1.48; previous revision: 1.47
done
Checking in widget/public/nsIMenu.h;
/cvsroot/mozilla/widget/public/nsIMenu.h,v  <--  nsIMenu.h
new revision: 1.37; previous revision: 1.36
done

Comment 31

[(not reading, please use [e-mail] instead)]

this fix may have caused a regresion, see bug #394515

colin is investigating.

Comment 32

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008011009 Firefox/3.0b3pre ID:2008011009 no assertion on testcase - Verified