Assertion failure: (((bool)(__builtin_expect(!!(!NS_FAILED_impl( options->GetOptionIndex(this, 0, true, &index))), 1)))), at dom/html/HTMLOptionElement.cpp:148 -> 147 MOZ_ALWAYS_TRUE(NS_SUCCEEDED( 148 options->GetOptionIndex(this, 0, true, &index))); This code seems to come from bug 903791.

That does not at all seems like the right fix.

Comment on attachment 8724769 [details] [diff] [review] crash.patch I agree. There should be no exception here unless the data structures involved are confused, and then the right fix is to not let them get into this confused state, no? What am I missing?

Comment on attachment 8724812 [details] [diff] [review] crash2.patch Does this lead to an observable behavior difference other than "don't crash on .index" access? If so, what difference?

without this patch, the testcase generates this dom: <select> <optgroup (g1)> <optgroup (g2)> <option (o2) /> </optgroup> <option (o1) /> </optgroup> </select> How you can see we add the optgroup (g2) before option (o2) and not before g1 as we should do.

OK, can you add a correctness test (non-crashtest) that verifies that the DOM ends up correct?

With tests.

Hmm. So I finally carefully read the testcase and the spec. The testcase is doing: select.add(g2, 0); in a slightly roundabout way. The spec says: Similarly, the add() method must act like its namesake method on that same options collection. which means we should do what https://html.spec.whatwg.org/multipage/infrastructure.html#dom-htmloptionscollection-add says. And that says: 4. If before is a node, then let reference be that node. Otherwise, if before is an integer, and there is a beforeth node in the collection, let reference be that node. Otherwise, let reference be null. in this case that means "reference" becomes o1, since HTMLOptionsCollection just contains the <options> of the <select> and its optgroups. Then we have: 5. If reference is not null, let parent be the parent node of reference. Otherwise, let parent be the select element on which the HTMLOptionsCollection is rooted. 7. Act as if the DOM insertBefore() method was invoked on the parent node, with element as the first argument and reference as the second argument. so the actual DOM produced by the testcase without this patch matches the current spec. Why did you decide it should be different? I put up a testcase for this behavior at https://jsfiddle.net/oxw7cnaq/ that tries to show what's going on. The behavior I see in browsers is: Firefox: Follows the current spec, for whatever that's worth; it leads to nested optgroups in a way that doesn't make sense in general. Chrome: Throws an exception. Safari: Throws an exception. IE11: Does what Firefox does. Edge 13: Does what Firefox does. So maybe we have room, for this particular case, for changing the spec to make more sense and not create nested optgroups?

And in particular, maybe we should in fact throw if the parent is not the <select> itself and an optgroup is being inserted? It makes sense to insert an optgroup at toplevel between options, and makes sense to insert an option at either toplevel or in an optgroup, but inserting an optgroup inside an optgroup does not make sense.

Comment on attachment 8725283 [details] [diff] [review] crash2.patch Whatever the outcome of the spec stuff, this doesn't look right; simply having optgroup children is NOT the right criterion here and would break the "insert an option inside an optgroup case. Which I really hope we have tests for... did you put this patch through try?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2d97a539e95&selectedJob=17476598 Yes. Green.

We could change the specification here to make that requirement. It generally makes sense to match Safari/Chrome these days, although throwing is usually less compatible.

> Yes. Green. Sounds like we need more tests, then, since the patch clearly has broken behavior in that case I mentioned...

bz, annevk, are you ok if in my next patch, add() throws an exception if we try to add an optGroup before something that is an optGroup?

I think that you should work with Anne to figure out what the spec should say here. Then implement the spec.

But as a separate matter, if you produce the DOM shown in comment 7 via pure appendChild calls (which you can totally do), what happens on the .index get?

We have exactly the same crash. I guess my first patch is ok with this case. Any reason why we don't want to throw exceptions in 'index'?

What is the security issue here, Andrea? I can't tell by skimming the bug.

> Any reason why we don't want to throw exceptions in 'index'? Uh... Yes. The spec doesn't say to throw any. Seriously, we're not just making this stuff up here in a vacuum. We should either implement the spec or have concrete reasons why the spec doesn't make sense (as in the add() case). Per spec, if the <option> is in a "list of options" then you return its index in that list, otherwise you return 0. In the DOM from comment 7, the o2 option is not in the list of options, so should return 0.

Arguably, HTMLOptionElement::GetSelect should return null in this case... I wonder why it does not.

Probably because we forgot to change it in bug 1214164.

GetSelect returns always something because in our case we have 2 nested OptGroup objects and our while loop passes them both and finds the select object.

> GetSelect returns always something Yes, and I'm saying that's wrong.

Comment on attachment 8725752 [details] [diff] [review] crash3.patch >+ HTMLSelectElement* select = HTMLSelectElement::FromContent(parent); You just crashed if parent is null. Please return null immediately if !parent, and add a test. >+ select = HTMLSelectElement::FromContent(parent->GetParent()); You just crashes if parent->GetParent() is null. Again, add a test. I think you just want to do: return HTMLSelectElement::FromContentOrNull(parent->GetParent()); and ditch the if/else bits. >+++ b/dom/html/crashtests/1250401.html That's good, but it would be good to have this test via explicit DOM construction, not select.add(), since we're talking about having that throw in this situation.... Also, this can probably be a web platform test asserting that o2.index == 0; would be better that way. Then again, since your mochitest tests all of this stuff, is this test really needed? >+++ b/dom/html/test/test_bug1250401.html Again, web platform test. >+ is(select.firstChild.children.length, 2, "g2 has 2 children"); >+ is(select.firstChild.children[0], g2, "g2 has 2 children: g2"); >+ is(select.firstChild.children[1], o1, "g2 has 2 children: o1"); No, g1 has two children for all of those. >+ is(select.firstChild.children.length, 2, "g2 has 2 children"); >+ is(select.firstChild.children[0], o1, "g2 has 2 children: o1"); >+ is(select.firstChild.children[1], g2, "g2 has 2 children: g1"); And here too. r=me with those fixed.

[Security approval request comment] How easily could an exploit be constructed based on the patch? On Debug build we crash because of a MOZ_ASSERT. On a non-debug build we return an non-assigned integer. I don't think this can be exposed. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not really. We just fix the option.index value. Which older supported branches are affected by this flaw? All. GetSelect() seems quite untouched since 2012. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Easy to backport. How likely is this patch to cause regressions; how much testing does it need? It's fully green on try, but as some previous comments prove, we don't have so much tests in this area. But still, I don't think it cause regressions: nested optGroup objects should not be so common.

>Which older supported branches are affected by this flaw? >All. GetSelect() seems quite untouched since 2012. Yes, but options inside nested optgroups used to be counted in the options list until bug 1214164. So afaict this bug is a regression introduced in Firefox 44. Have you checked whether it shows up in ESR38, for instance? I expect that it does not and that backporting this to there would actually break other things.

You still need tests for .index on an option with no parent at all, right?

Oh, you totally do; you didn't even fix that part in the patch!

Approval Request Comment [Feature/regressing bug #]: bug 1214164 [User impact if declined]: a random value can be returned by option.index. Or a crash in debug builds. [Describe test coverage new/current, TreeHerder]: test included [Risks and why]: none [String/UUID change made/needed]: none

"random value" isn't actually true; we'll return 0 if the assertion fails.

(In reply to :Ms2ger from comment #33) > "random value" isn't actually true; we'll return 0 if the assertion fails. Right, because index is initialized to the default value.

Comment on attachment 8726107 [details] [diff] [review] crash3.patch >+ is(o1.index, 0, "o2.index should be 1"); >+ is(o1.index, 0, "o2.index should be 0"); >+ is(o1.index, 0, "o2.index should be 0"); >+ is(o1.index, 0, "o2.index should be 0"); Fix the messages, please.

Regression from 44, tracking and marking affected

https://hg.mozilla.org/mozilla-central/rev/8b0b020feaf6 https://hg.mozilla.org/mozilla-central/rev/41e6e56e1186 https://hg.mozilla.org/mozilla-central/rev/2ab0db7c01f8 https://hg.mozilla.org/mozilla-central/rev/d64e1ec71f9a

Comment on attachment 8726202 [details] [diff] [review] crash3.patch This didn't make it in before the merge and I'm about to start beta 1. We can take it for beta 2 though. Switching the request flag to beta.

Too late for 45.

bz, baku, al, do you have strong opinions about uplifting this to 46? Is this a problem we know people are hitting now? Do you think the risk of creating new problems (crashes) in 46 is worth the security benefit from pushing this fix to 46?

I think the risk from this patch is pretty low, but I also don't think it's super-critical to fix on 46....

I don't see how this is a security bug, it's just a wrong result.

Comment on attachment 8726202 [details] [diff] [review] crash3.patch If this is just a crash in debug builds I'd rather let it ride with 47, to decrease risk in beta.