877910 - opening railsdiff crashes Firefox in mozilla::dom::HTMLOptionsCollection::NamedItem @ JSAutoCompartment::JSAutoCompartment

Status: RESOLVED FIXED

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

Description

[Sören Hentzschel]

STR:

1. open http://railsdiff.org/html/v3.2.13-v4.0.0.rc1.html with Nightly or Aurora and enabled JavaScript

Result:

Firefox crashes

Crash Signature:
[@ JSAutoCompartment::JSAutoCompartment(JSContext*, JSObject*) ]

Crash Report:
https://crash-stats.mozilla.com/report/index/bp-9fc8445c-f0a6-4958-a07b-ccdf62130530

Tested with:

Mac OS X 10.8.3 Nightly (clean profile): crash
Mac OX X 10.8.3 Aurora (clean profile): crash
Mac OS X 10.8.3 Beta (clean profile): no crash
Mac OS X 10.8.3 Stable (clean profile): no crash
Windows 8 Nightly: crash
Windows 8 Stable: no crash

Comment 1

[Boris Zbarsky [:bzbarsky]]

Thank you for the bug report!

The relevant stack bit is:

#4  0x000000010179ab45 in JSAutoCompartment::JSAutoCompartment (this=0x7fff5fbf0eb8, cx=0x117296400, target=0x0) at jsapi.cpp:1503
#5  0x0000000103a54786 in mozilla::dom::HTMLOptionsCollection::NamedItem (this=0x120410b80, cx=0x117296400, name=@0x7fff5fbf0fc8, error=@0x7fff5fbf0fa0) at HTMLOptionsCollection.cpp:293
#6  0x00000001053274e7 in mozilla::dom::HTMLSelectElement::NamedItem (this=0x1213743e0, aCx=0x117296400, aName=@0x7fff5fbf0fc8, aRv=@0x7fff5fbf0fa0) at HTMLSelectElement.h:212

HTMLOptionsCollection::NamedItem assumes it's called directly from script an hence that the options collection is JS-wrapped, but in this case it's C++ code calling it.

We need to either make sure mOptions is wrapped or we need to do the return-value wrapping ourselves.

Simple testcase:

  data:text/html,<select><option id="foo"><script>alert(document.querySelector("select").namedItem("foo"))</script>

Comment 2

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

Attachment: Patch v1

This is probably the easiest solution.

Comment 3

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

Attachment: Patch v1.1

In case you prefer a patch that builds.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 756507 [details] [diff] [review]
Patch v1.1

>+/* static */ HTMLOptionElement*

That comment is wrong.  Please remove it.

>+  HTMLOptionElement? namedItem(DOMString name);

I think we should get the spec adjusted like this as well.

r=me; I like this.  ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and I assume this applies fine to Aurora, right?

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/b7344a8ee6f2

Comment 7

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

Attachment: Patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 841488
User impact if declined: null pointer dereference crashes
Testing completed (on m-c, etc.): on m-c, passes tests, manually checked original site
Risk to taking this patch (and alternatives if risky): low risk. Alternative is backing out, but that's probably riskier.
String or IDL/UUID changes made by this patch: None.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a88c28367150

Comment 9

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

Target milestone is when it landed on trunk