1308862 - AVR:NULL+0x7C d87.353 @ firefox.exe!xul.dll!mozilla::dom::HTMLTrackElement::HTMLTrackElement

Status: RESOLVED FIXED

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

Description

[SkyLined]

Attachment: repro.svg

Embedding an HTML `track` element in an svg image that is rendered using an svg `feImage` element causes a NULL pointer.

repro:
<track xmlns="http://www.w3.org/1999/xhtml"><feImage xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" xlink:href='?'>

When the repro is loaded, the track element is not yet important, but the feImage element will cause the repro to load itself as an image. When this second instance is loaded, the track element will cause a NULL pointer access violation.

Comment 1

[SkyLined]

Attachment: BugId report

Added BugId report, it contains more useful information about the crash, such as the location in the source.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Do you have a crash id for this?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

 0x00007fffe7dcce32 in mozilla::dom::HTMLTrackElement::HTMLTrackElement(already_AddRefed<mozilla::dom::NodeInfo>&) (this=0x7fffbd379060, aNodeInfo=...)
    at /home/smaug/mozilla/hg/inbound2/dom/html/HTMLTrackElement.cpp:133
#1  0x00007fffe7dccd22 in NS_NewHTMLTrackElement(already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser) (aNodeInfo=<unknown type in /home/smaug/mozilla/hg/inbound2/ff_build_opt/dist/bin/libxul.so, CU 0x9cb7a04, DIE 0x9d50714>, aFromParser=<optimized out>) at /home/smaug/mozilla/hg/inbound2/dom/html/HTMLTrackElement.cpp:53
#2  0x00007fffe7dea183 in NS_NewHTMLElement(mozilla::dom::Element**, already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser, nsAString_internal const*) (aNodeType=129, aNodeInfo=<unknown type in /home/smaug/mozilla/hg/inbound2/ff_build_opt/dist/bin/libxul.so, CU 0x9daa6a1, DIE 0x9e72ea5>, aFromParser=mozilla::dom::FROM_PARSER_NETWORK) at /home/smaug/mozilla/hg/inbound2/dom/html/nsHTMLContentSink.cpp:289
#3  0x00007fffe7dea183 in NS_NewHTMLElement(mozilla::dom::Element**, already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser, nsAString_internal const*) (aResult=0x7fffffffb9e0, aNodeInfo=<optimized out>, aFromParser=mozilla::dom::FROM_PARSER_NETWORK, aIs=<optimized out>) at /home/smaug/mozilla/hg/inbound2/dom/html/nsHTMLContentSink.cpp:270
#4  0x00007fffe736fc26 in NS_NewElement(mozilla::dom::Element**, already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser, nsAString_internal const*) (aResult=0x7fffffffb9e0, aNodeInfo=<optimized out>, aFromParser=(mozilla::dom::FROM_PARSER_XSLT | unknown: 4139778112), aIs=0x17) at /home/smaug/mozilla/hg/inbound2/dom/base/nsNameSpaceManager.cpp:177
#5  0x00007fffe8306325 in nsXMLContentSink::CreateElement(char16_t const**, unsigned int, mozilla::dom::NodeInfo*, unsigned int, nsIContent**, bool*, mozilla::dom::FromParser) (this=
    0x7fffa923b000, aAtts=<optimized out>, aAttsCount=<optimized out>, aNodeInfo=0x7fffa66e38d0, aLineNumber=1, aResult=0x7fffffffba78, aAppendContent=0x7fffffffba87, aFromParser=mozilla::dom::FROM_PARSER_NETWORK) at /home/smaug/mozilla/hg/inbound2/dom/xml/nsXMLContentSink.cpp:465
#6  0x00007fffe83074dd in nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int, bool) (this=0x7fffa923b000, aName=<optimized out>, aAtts=0x7fffb6a16c00, aAttsCount=1, aLineNumber=1, aInterruptable=true) at /home/smaug/mozilla/hg/inbound2/dom/xml/nsXMLContentSink.cpp:951

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I downloaded the testcase and opened it from file system.

We crash in http://searchfox.org/mozilla-central/rev/3e03a4064eb585d96f28023785a5c242969878a6/dom/html/HTMLTrackElement.cpp#133 because window is null.
parentObject seems to be SandboxPrivate*.


Longer stack
#0  0x00007fffe1822ec4 in nsCOMPtr<nsPIDOMWindowInner>::operator->() const (this=0x7fffffffa448) at /home/smaug/mozilla/hg/inbound2/ff_build/dist/include/nsCOMPtr.h:755
#1  0x00007fffe46c72e8 in mozilla::dom::HTMLTrackElement::HTMLTrackElement(already_AddRefed<mozilla::dom::NodeInfo>&) (this=0x7fffb20b75c0, aNodeInfo=...)
    at /home/smaug/mozilla/hg/inbound2/dom/html/HTMLTrackElement.cpp:133
#2  0x00007fffe46c6d40 in NS_NewHTMLTrackElement(already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser) (aNodeInfo=<unknown type in /home/smaug/mozilla/hg/inbound2/ff_build/dist/bin/libxul.so, CU 0x75c02f6, DIE 0x765d6e5>, aFromParser=mozilla::dom::FROM_PARSER_NETWORK) at /home/smaug/mozilla/hg/inbound2/dom/html/HTMLTrackElement.cpp:53
#3  0x00007fffe4701fc7 in CreateHTMLElement(unsigned int, already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser) (aNodeType=129, aNodeInfo=<unknown type in /home/smaug/mozilla/hg/inbound2/ff_build/dist/bin/libxul.so, CU 0x768f6f9, DIE 0x7753d6e>, aFromParser=mozilla::dom::FROM_PARSER_NETWORK) at /home/smaug/mozilla/hg/inbound2/dom/html/nsHTMLContentSink.cpp:289
#4  0x00007fffe4701e9b in NS_NewHTMLElement(mozilla::dom::Element**, already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser, nsAString_internal const*) (aResult=0x7fffffffa748, aNodeInfo=<unknown type in /home/smaug/mozilla/hg/inbound2/ff_build/dist/bin/libxul.so, CU 0x768f6f9, DIE 0x7753bfa>, aFromParser=mozilla::dom::FROM_PARSER_NETWORK, aIs=0x0)
    at /home/smaug/mozilla/hg/inbound2/dom/html/nsHTMLContentSink.cpp:270
#5  0x00007fffe315bb09 in NS_NewElement(mozilla::dom::Element**, already_AddRefed<mozilla::dom::NodeInfo>&&, mozilla::dom::FromParser, nsAString_internal const*) (aResult=0x7fffffffa748, aNodeInfo=<unknown type in /home/smaug/mozilla/hg/inbound2/ff_build/dist/bin/libxul.so, CU 0x4fde26a, DIE 0x5080bb0>, aFromParser=mozilla::dom::FROM_PARSER_NETWORK, aIs=0x0)
    at /home/smaug/mozilla/hg/inbound2/dom/base/nsNameSpaceManager.cpp:177
#6  0x00007fffe52772c3 in nsXMLContentSink::CreateElement(char16_t const**, unsigned int, mozilla::dom::NodeInfo*, unsigned int, nsIContent**, bool*, mozilla::dom::FromParser) (this=
    0x7fffb1f47000, aAtts=0x7fffb75f0800, aAttsCount=1, aNodeInfo=0x7fffb2baf400, aLineNumber=1, aResult=0x7fffffffa910, aAppendContent=0x7fffffffa91f, aFromParser=mozilla::dom::FROM_PARSER_NETWORK)
    at /home/smaug/mozilla/hg/inbound2/dom/xml/nsXMLContentSink.cpp:465
#7  0x00007fffe52792a9 in nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int, bool) (this=0x7fffb1f47000, aName=0x7fffba2cac10 u"http://www.w3.org/1999/xhtml\xfffftrack", aAtts=0x7fffb75f0800, aAttsCount=1, aLineNumber=1, aInterruptable=true) at /home/smaug/mozilla/hg/inbound2/dom/xml/nsXMLContentSink.cpp:951
#8  0x00007fffe5278f19 in nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int) (this=0x7fffb1f47000, aName=0x7fffba2cac10 u"http://www.w3.org/1999/xhtml\xfffftrack", aAtts=0x7fffb75f0800, aAttsCount=2, aLineNumber=1) at /home/smaug/mozilla/hg/inbound2/dom/xml/nsXMLContentSink.cpp:909
#9  0x00007fffe5279885 in non-virtual thunk to nsXMLContentSink::HandleStartElement(char16_t const*, char16_t const**, unsigned int, unsigned int) ()
    at /home/smaug/mozilla/hg/inbound2/dom/xml/nsXMLContentSink.cpp:904
#10 0x00007fffe27c1044 in nsExpatDriver::HandleStartElement(char16_t const*, char16_t const**) (this=0x7fffa42f7fc0, aValue=0x7fffba2cac10 u"http://www.w3.org/1999/xhtml\xfffftrack", aAtts=0x7fffb75f0800)
    at /home/smaug/mozilla/hg/inbound2/parser/htmlparser/nsExpatDriver.cpp:380
#11 0x00007fffe27c5bad in Driver_HandleStartElement(void*, char16_t const*, char16_t const**) (aUserData=0x7fffa42f7fc0, aName=0x7fffba2cac10 u"http://www.w3.org/1999/xhtml\xfffftrack", aAtts=0x7fffb75f0800)
    at /home/smaug/mozilla/hg/inbound2/parser/htmlparser/nsExpatDriver.cpp:67
#12 0x00007fffe6b83a7c in doContent (parser=0x7fffbcda2800, startTagLevel=0, enc=0x7fffeae970f8 <little2_encoding_ns>, s=0x7fffb2b7a730 "<", end=0x7fffb2b7a854 "\344\344\344", <incomplete sequence \344>, nextPtr=0x7fffffffb048, haveMore=1 '\001') at /home/smaug/mozilla/hg/inbound2/parser/expat/lib/xmlparse.c:2442
#13 0x00007fffe6b80d01 in contentProcessor (parser=0x7fffbcda2800, start=0x7fffb2b7a730 "<", end=0x7fffb2b7a854 "\344\344\344", <incomplete sequence \344>, endPtr=0x7fffffffb048)
    at /home/smaug/mozilla/hg/inbound2/parser/expat/lib/xmlparse.c:2098
#14 0x00007fffe6b7db05 in doProlog (parser=0x7fffbcda2800, enc=0x7fffeae970f8 <little2_encoding_ns>, s=0x7fffb2b7a730 "<", end=0x7fffb2b7a854 "\344\344\344", <incomplete sequence \344>, tok=29, next=0x7fffb2b7a730 "<", nextPtr=0x7fffffffb048, haveMore=1 '\001') at /home/smaug/mozilla/hg/inbound2/parser/expat/lib/xmlparse.c:4078
#15 0x00007fffe6b7ce43 in prologProcessor (parser=0x7fffbcda2800, s=0x7fffb2b7a730 "<", end=0x7fffb2b7a854 "\344\344\344", <incomplete sequence \344>, nextPtr=0x7fffffffb048)
    at /home/smaug/mozilla/hg/inbound2/parser/expat/lib/xmlparse.c:3812
#16 0x00007fffe6b7cc10 in prologInitProcessor (parser=0x7fffbcda2800, s=0x7fffb2b7a730 "<", end=0x7fffb2b7a854 "\344\344\344", <incomplete sequence \344>, nextPtr=0x7fffffffb048)
    at /home/smaug/mozilla/hg/inbound2/parser/expat/lib/xmlparse.c:3629
#17 0x00007fffe6b7b7a2 in MOZ_XML_Parse (parser=0x7fffbcda2800, s=0x7fffb2b7a730 "<", len=292, isFinal=0) at /home/smaug/mozilla/hg/inbound2/parser/expat/lib/xmlparse.c:1530
#18 0x00007fffe27c42f1 in nsExpatDriver::ParseBuffer(char16_t const*, unsigned int, bool, unsigned int*) (this=0x7fffa42f7fc0, aBuffer=0x7fffb2b7a730 u"<track xmlns=\"http://www.w3.org/1999/xhtml\"><feImage xmlns=\"http://www.w3.org/2000/svg\" xmlns:xlink=\"http://www.w3.org/1999/xlink\" xlink:href='?'>", aLength=146, aIsFinal=false, aConsumed=0x7fffffffb24c)
    at /home/smaug/mozilla/hg/inbound2/parser/htmlparser/nsExpatDriver.cpp:1012
#19 0x00007fffe27c4a05 in nsExpatDriver::ConsumeToken(nsScanner&, bool&) (this=0x7fffa42f7fc0, aScanner=..., aFlushTokens=@0x7fffffffb523: false)
    at /home/smaug/mozilla/hg/inbound2/parser/htmlparser/nsExpatDriver.cpp:1110
#20 0x00007fffe27c555f in non-virtual thunk to nsExpatDriver::ConsumeToken(nsScanner&, bool&) () at /home/smaug/mozilla/hg/inbound2/parser/htmlparser/nsExpatDriver.cpp:1043
#21 0x00007fffe27cb26b in nsParser::Tokenize(bool) (this=0x7fffb74d8740, aIsFinalChunk=false) at /home/smaug/mozilla/hg/inbound2/parser/htmlparser/nsParser.cpp:1944
#22 0x00007fffe27c9d89 in nsParser::ResumeParse(bool, bool, bool) (this=0x7fffb74d8740, allowIteration=true, aIsFinalChunk=false, aCanInterrupt=true)
    at /home/smaug/mozilla/hg/inbound2/parser/htmlparser/nsParser.cpp:1463
#23 0x00007fffe27cbb99 in nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (this=0x7fffb74d8740, request=0x7fffb1d30360, aContext=0x0, pIStream=0x7fffc70919d0, sourceOffset=0, aLength=146) at /home/smaug/mozilla/hg/inbound2/parser/htmlparser/nsParser.cpp:1842
#24 0x00007fffe27cbe9c in non-virtual thunk to nsParser::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) ()
    at /home/smaug/mozilla/hg/inbound2/parser/htmlparser/nsParser.cpp:1779
#25 0x00007fffe2d651b4 in mozilla::image::SVGDocumentWrapper::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (this=0x7fffc67e31f0, aRequest=0x7fffb1d30360, ctxt=0x0, inS---Type <return> to continue, or q <return> to quit---
tr=0x7fffc70919d0, sourceOffset=0, count=146) at /home/smaug/mozilla/hg/inbound2/image/SVGDocumentWrapper.cpp:229
#26 0x00007fffe2d6d1b0 in mozilla::image::VectorImage::OnDataAvailable(nsIRequest*, nsISupports*, nsIInputStream*, unsigned long, unsigned int) (this=
    0x7fffb2da1820, aRequest=0x7fffb1d30360, aCtxt=0x0, aInStr=0x7fffc70919d0, aSourceOffset=0, aCount=146) at /home/smaug/mozilla/hg/inbound2/image/VectorImage.cpp:1281


So, we're loading something here as a svg-image, not as a document, and get the sandbox because of that? (I'm not familiar with that setup. dholbert might be)

Comment 5

[Daniel Holbert [:dholbert]]

(This might be related to bug 1297816 -- startup crash in Thunderbird, with nothing SVG/Vector-related in the stack.)

I don't know what a SandboxPrivate is.  IIRC we might not create a script global object inside of SVG-as-an-image (because scripting is disabled) -- maybe that's part of what's violating some assumptions here?  Similarly, scripting is disabled in the email-viewer in Thunderbird. So maybe this bug only reproduces in contexts where scripting is disabled?

bechen, could you take a look at this?

Comment 6

[Daniel Holbert [:dholbert]]

mozregression pins this as a regression from bug 1295097 (unsurprising, given hg blame on the code where we're crashing).

Range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ca7974ddccc11ec992834b8563d381d16b56ee42&tochange=8cc4907776c85b5f57c1778bdd5b2a4ad8eb8055

[Tracking Requested - why for this release]: new regression (crash). Also can manifest as a Thunderbird startup crash.

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (fixed XML errors): Crashes Nightly, when viewed as a local file

I got an XML error page when loading the first testcase, I think due to missing close tags.

I've fixed those in this modified testcase, and I've added an <svg> outer tag, for good measure.  (This is the version of the testcase that I used with mozregression, locally.)

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8799442 [details]
testcase 2 (fixed XML errors): Crashes Nightly, when viewed as a local file

(Like smaug in comment 4, I can only reproduce this when I view the testcase as a local file, for some reason -- perhaps due to differences in security, or perhaps to coming out on the other side of a network->parser->document race-condition.  In any case: updating testcase title, to make that clear.)

Comment 10

[Benjamin Chen [:bechen]]

Attachment: Bug 1308862 - Null check for the innerWindow.

Review commit: https://reviewboard.mozilla.org/r/85164/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85164/

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8800171 [details]
Bug 1308862 - Null check for the innerWindow.

https://reviewboard.mozilla.org/r/85164/#review83736

Comment 12

[Daniel Holbert [:dholbert]]

Drive-by questions:
 - Do we know *why* we end up with a SandboxPrivate* as our document's parentObject? (per comment 4)
 - Is it OK that we've got a SandboxPrivate* as our document's parentObject, when we create this HTMLTrackElement? (Does that obviously violate any other assumptions?)
 - In that scenario where we have a SandboxPrivate*, is it fine/expected that we end up *without* a WindowDestroyObserver? (since this patch is making us skip that instantiation) i.e. is there any cleanup work that we depend on it doing, which we'll missing out on?  Or is its cleanup work really only needed in the case where we have a nsPIDOMWindowInner, and there's no need for any such work in the SandboxPrivate* case?

(These sorts of questions should always be considered/explained for a change that adds a null-check like this, IMO -- to be sure the null-check is actually the right fix, and it's not skipping over code that we actually need, and we're not simply shifting the problem to a different place in the code.)

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #12)
> Drive-by questions:
>  - Do we know *why* we end up with a SandboxPrivate* as our document's parentObject? (per comment 4)

(I'm guessing the answer to this part is "because scripts are disabled". I'm less clear on the other questions; perhaps the parentObject & WindowDestroyObserver are only needed for interacting / preventing-interaction with JS provided by the author, in which case maybe it's fine to disregard it in cases where scripts are disabled? *shrug*)

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

sandbox is probably from http://searchfox.org/mozilla-central/rev/cd1be634c9309c7fc99a3fde67dd44d343875f60/dom/base/nsDocument.cpp#1930
Normally we override that then with some window object, but apparently we're using svg-as-image here.

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Please also file a bug to address the concern of comment 12.

Comment 16

[Benjamin Chen [:bechen]]

(In reply to Daniel Holbert [:dholbert] (PTO til Monday 10/17) from comment #12)
> Drive-by questions:
>  - Do we know *why* we end up with a SandboxPrivate* as our document's
> parentObject? (per comment 4)
>  - Is it OK that we've got a SandboxPrivate* as our document's parentObject,
> when we create this HTMLTrackElement? (Does that obviously violate any other
> assumptions?)
Not familiar with the SandboxPrivate mechanism, but I try to set the break point at the constructor of HTMLTrackElement and found the HTMLTrackElement had been created twice, the first is normal with a innerwindow-id, the second comes the SandboxPrivate.
In addition, I don't know why put the trackElement in svg, the trackElement doesn't work.

>  - In that scenario where we have a SandboxPrivate*, is it fine/expected
> that we end up *without* a WindowDestroyObserver? (since this patch is
> making us skip that instantiation) i.e. is there any cleanup work that we
> depend on it doing, which we'll missing out on?  Or is its cleanup work
> really only needed in the case where we have a nsPIDOMWindowInner, and
> there's no need for any such work in the SandboxPrivate* case?
> 
> (These sorts of questions should always be considered/explained for a change
> that adds a null-check like this, IMO -- to be sure the null-check is
> actually the right fix, and it's not skipping over code that we actually
> need, and we're not simply shifting the problem to a different place in the
> code.)

I think it is fine(not perfect) to not using WindowDestroyObserver. At bug 1286751, I presume only few links in TrackElement make the memory leak(somehow the necko has no response so I use WindowDestroyObserver to close the channel manually).

Comment 17

[Benjamin Chen [:bechen]]

Just verify the patch with bug 1286751, the patch won't make the regression because the TrackElement doesn't have a MediaElement parent, so the TrackElement won't fetch data from netwrok.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/905f1bc96fb6
Null check for the innerWindow. r=jwwang

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/905f1bc96fb6

Comment 20

[Benjamin Chen [:bechen]]

Attachment: bug1308862.aurora.patch

Approval Request Comment
[Feature/regressing bug #]: 1295097
[User impact if declined]: crash if open a svg file embed a trackElement.
[Describe test coverage new/current, TreeHerder]: none
[Risks and why]: low risk, fix is simple.
[String/UUID change made/needed]: none

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8801981 [details] [diff] [review]
bug1308862.aurora.patch

Fix a crash related to svg. Take it in 51 aurora.

Comment 22

[Gerry Chang [:gchang]]

Track 51+ as this is a crash and regression.

Comment 23

[Carsten Book [:Tomcat]]

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