604807 - Crash [@ nsHTMLSelectElement::GetOptionIndex] with QI to nsISelectElement

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Boris Zbarsky [:bzbarsky]]

This is crap left over from XBL form controls.  We should make this interface noscript as a quick fix, imo.

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I have patches to remove nsISelectElement altogether, but I guess that won't fly for 2.0.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Probably not.  Want to do a patch per comment 1?  ;)

Comment 4

[Damon Sicore (:damons)]

We have a systematic problem we need to fix that will resolve this issue:  bug 605271.  It's a blocker, so minusing this one.

Comment 5

[Daniel Veditz [:dveditz]]

It doesn't seem to crash the 1.9.2 branch, do we still want to make the interface [noscript] anyway just in case? Any others in the same family we should do the same to?

Comment 6

[Boris Zbarsky [:bzbarsky]]

We should make this noscript on branch, yes.  imo.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is fixed by Bug 605271.

Comment 8

[Daniel Veditz [:dveditz]]

bug 605271 is a lot more than making the iface [noscript]. Is it necessary (and if so, is it a safe change for the branch?) or is there a quick IDL fix along the lines of comment 1?

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Stealing.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 488440 [details] [diff] [review]
Patch v1

r=me

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Do we want/need to take that on 2.0?

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch for checkin

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 488570 [details] [diff] [review]
Patch for checkin

Four addons tried to fool my search by doing this:
  const nsISelectElement = Components.interfaces.nsIDOMHTMLSelectElement;

(and then using nsISelectElement) but none use the real nsISelectElement so we look OK going this route.

Approved for 1.9.2.13 and 1.9.1.16, a=dveditz for release-drivers

Comment 14

[Daniel Veditz [:dveditz]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f43d4cdbe257
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/88e47d219f9d

Comment 15

[Daniel Veditz [:dveditz]]

Leaving open for trunk until bug 605271 has landed (or this patch is landed in the interim?). I believe checkin-needed referred only to the branches so I'm removing that keyword, but if we want this interim patch meanwhile it could be re-added.

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We should just land this patch on trunk.  Bug 605271 isn't going to make it.

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This should be blocking since 605271 isn't anymore.

Comment 18

[Mounir Lamouri (:mounir)]

Jonas, Johnny, this should block, isn't it?

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes

Comment 20

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/a01537c0bf19

Comment 21

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if  you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Readding everything that got stripped ...