Fix a browser crasher with negative index set on select popups.

Fix a browser crasher with negative index set on select popups.

BUG=63774
TEST=Visit http://vescam.com/select_crasher.html and try all the selects
     in that page.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67969

[Jay Civelli, 2010-12-01 01:08:51]

http://codereview.chromium.org/5302009/diff/1/webkit/glue/webmenurunner_mac.mm
File webkit/glue/webmenurunner_mac.mm (right):

http://codereview.chromium.org/5302009/diff/1/webkit/glue/webmenurunner_mac.m...
webkit/glue/webmenurunner_mac.mm:131: // Set up the button cell, converting to
NSView coordinates. The menu is
I am not sure how to test that case? Should I just hack gNewNSMenuAPI to be
false?

[Scott Hess - ex-Googler, 2010-12-01 01:29:51]

Thanks for working on this!

http://codereview.chromium.org/5302009/diff/1/webkit/glue/webmenurunner_mac.mm
File webkit/glue/webmenurunner_mac.mm (right):

http://codereview.chromium.org/5302009/diff/1/webkit/glue/webmenurunner_mac.m...
webkit/glue/webmenurunner_mac.mm:95: [menuItem setTag:[menu_ numberOfItems] -
1];
You can safely make this unconditional.

http://codereview.chromium.org/5302009/diff/1/webkit/glue/webmenurunner_mac.m...
webkit/glue/webmenurunner_mac.mm:124: [selectedItem setState:NSOnState];
If I understand this correctly, what you want is the actual item, if possible,
else the item at 0?  This seems sort of convoluted for that.  How about:

NSMenuItem* selectedItem;
if (index < 0 || index > [menu_ numberOfItems]) {
  selectedItem = [menu_ itemAtIndex:0];
} else {
  selectedItem = [menu_ itemAtIndex:index];
  [selectedItem setState:NSOnState];
}

Hmm.  Or even:

NSMenuItem* selectedItem = [menu_ itemWithTag:index];
if (selectedItem) {
  [selectedItem setState:NSOnState];
} else {
  selectedItem = [menu_ itemWithTag:0];  // In case menu is empty.
}

http://codereview.chromium.org/5302009/diff/1/webkit/glue/webmenurunner_mac.m...
webkit/glue/webmenurunner_mac.mm:131: // Set up the button cell, converting to
NSView coordinates. The menu is
On 2010/12/01 01:08:52, Jay Civelli wrote:
> I am not sure how to test that case? Should I just hack gNewNSMenuAPI to be
> false?

OMG.

I guess that's about your only option.  Or run on a 10.5 system.  Or just use
the older code everywhere (which is what our UI code does), though that's too
big a change to consider for 9.0 I think.

If you take my -itemWithTag: idea, consider -selectItemWithTag: below, to be
consistent.

[Jay Civelli, 2010-12-01 02:07:13]

http://codereview.chromium.org/5302009/diff/1/webkit/glue/webmenurunner_mac.mm
File webkit/glue/webmenurunner_mac.mm (right):

http://codereview.chromium.org/5302009/diff/1/webkit/glue/webmenurunner_mac.m...
webkit/glue/webmenurunner_mac.mm:95: [menuItem setTag:[menu_ numberOfItems] -
1];
On 2010/12/01 01:29:51, shess wrote:
> You can safely make this unconditional.

Done.

http://codereview.chromium.org/5302009/diff/1/webkit/glue/webmenurunner_mac.m...
webkit/glue/webmenurunner_mac.mm:124: [selectedItem setState:NSOnState];
On 2010/12/01 01:29:51, shess wrote:
> If I understand this correctly, what you want is the actual item, if possible,
> else the item at 0?  This seems sort of convoluted for that.  How about:
> 
> NSMenuItem* selectedItem;
> if (index < 0 || index > [menu_ numberOfItems]) {
>   selectedItem = [menu_ itemAtIndex:0];
> } else {
>   selectedItem = [menu_ itemAtIndex:index];
>   [selectedItem setState:NSOnState];
> }
> 
> Hmm.  Or even:
> 
> NSMenuItem* selectedItem = [menu_ itemWithTag:index];
> if (selectedItem) {
>   [selectedItem setState:NSOnState];
> } else {
>   selectedItem = [menu_ itemWithTag:0];  // In case menu is empty.
> }
Good idea. Done.

http://codereview.chromium.org/5302009/diff/1/webkit/glue/webmenurunner_mac.m...
webkit/glue/webmenurunner_mac.mm:131: // Set up the button cell, converting to
NSView coordinates. The menu is
On 2010/12/01 01:29:51, shess wrote:
> On 2010/12/01 01:08:52, Jay Civelli wrote:
> > I am not sure how to test that case? Should I just hack gNewNSMenuAPI to be
> > false?
> 
> OMG.
> 
> I guess that's about your only option.  Or run on a 10.5 system.  Or just use
> the older code everywhere (which is what our UI code does), though that's too
> big a change to consider for 9.0 I think.
> 
> If you take my -itemWithTag: idea, consider -selectItemWithTag: below, to be
> consistent.

OK, I just hacked the value.
The API for NSPopUpButtonCell is slightly different, so I cannot have
consistency with the other case. Interestingly, -1 is OK for selectItemAtIndex,
so we only have to take care of out of bounds indeces.

[Scott Hess - ex-Googler, 2010-12-01 02:53:36]

lgtm

http://codereview.chromium.org/5302009/diff/5001/webkit/glue/webmenurunner_ma...
File webkit/glue/webmenurunner_mac.mm (right):

http://codereview.chromium.org/5302009/diff/5001/webkit/glue/webmenurunner_ma...
webkit/glue/webmenurunner_mac.mm:134: index = -1;  // -1 in selectItemAtIndex
means no selection.
What I meant down here was that -selectItemWithTag: with index too big or too
small would automatically work (because no item would be so tagged).  It can't
be completely consistent with the above code, but the style is similar.

[Jay Civelli, 2010-12-01 16:58:24]

http://codereview.chromium.org/5302009/diff/5001/webkit/glue/webmenurunner_ma...
File webkit/glue/webmenurunner_mac.mm (right):

http://codereview.chromium.org/5302009/diff/5001/webkit/glue/webmenurunner_ma...
webkit/glue/webmenurunner_mac.mm:134: index = -1;  // -1 in selectItemAtIndex
means no selection.
On 2010/12/01 02:53:36, shess wrote:
> What I meant down here was that -selectItemWithTag: with index too big or too
> small would automatically work (because no item would be so tagged).  It can't
> be completely consistent with the above code, but the style is similar.

Ah, I see. Done.

[Scott Hess - ex-Googler, 2010-12-01 18:14:37]

lgtm!