Closed Bug 1383780 Opened 7 years ago Closed 7 years ago

Stylo: Crash [@nsIContent::GetBaseURIForStyleAttr()] in /home/worker/workspace/build/src/dom/base/FragmentOrElement.cpp:465

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: jkratzer, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-nullptr, testcase)

Crash Data

Attachments

(3 files)

trigger.html
958 bytes, text/html
Details
Bug 1383780 - Null-check return value of SVGUseElement::GetContentURLData before returning.
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1383780 - Avoid adding anonymous editor element into non-HTML element.
59 bytes, text/x-review-board-request
masayuki
: review+
Details
Attached file trigger.html — 
Testcase found while fuzzing mozilla-central rev 20170722-c22502562670.

ASAN:DEADLYSIGNAL
=================================================================
==24274==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x7f9882b3fb4a bp 0x7ffd45043910 sp 0x7ffd45043820 T0)
==24274==The signal is caused by a READ memory access.
==24274==Hint: address points to the zero page.
    #0 0x7f9882b3fb49 in nsIContent::GetBaseURIForStyleAttr() const /home/worker/workspace/build/src/dom/base/FragmentOrElement.cpp:465
    #1 0x7f9886bfb1b6 in nsDOMCSSAttributeDeclaration::GetCSSParsingEnvironment(nsDOMCSSDeclaration::CSSParsingEnvironment&) /home/worker/workspace/build/src/layout/style/nsDOMCSSAttrDeclaration.cpp:172:37
    #2 0x7f9886bfc87f in ModifyDeclaration<(lambda at /home/worker/workspace/build/src/layout/style/nsDOMCSSDeclaration.cpp:364:5), (lambda at /home/worker/workspace/build/src/layout/style/nsDOMCSSDeclaration.cpp:370:5)> /home/worker/workspace/build/src/layout/style/nsDOMCSSDeclaration.cpp:336:5
    #3 0x7f9886bfc87f in nsDOMCSSDeclaration::ParsePropertyValue(nsCSSPropertyID, nsAString const&, bool) /home/worker/workspace/build/src/layout/style/nsDOMCSSDeclaration.cpp:363
    #4 0x7f9886781e73 in mozilla::ChangeStyleTransaction::DoTransaction() /home/worker/workspace/build/src/editor/libeditor/ChangeStyleTransaction.cpp:196:19
    #5 0x7f988695da5b in DoTransaction /home/worker/workspace/build/src/editor/txmgr/nsTransactionItem.cpp:156:26
    #6 0x7f988695da5b in nsTransactionManager::BeginTransaction(nsITransaction*, nsISupports*) /home/worker/workspace/build/src/editor/txmgr/nsTransactionManager.cpp:661
    #7 0x7f988695d2c3 in nsTransactionManager::DoTransaction(nsITransaction*) /home/worker/workspace/build/src/editor/txmgr/nsTransactionManager.cpp:74:8
    #8 0x7f9886782a80 in mozilla::EditorBase::DoTransaction(nsITransaction*) /home/worker/workspace/build/src/editor/libeditor/EditorBase.cpp:748:20
    #9 0x7f98867830fd in SetCSSProperty /home/worker/workspace/build/src/editor/libeditor/CSSEditUtils.cpp:463:23
    #10 0x7f98867830fd in mozilla::CSSEditUtils::SetCSSPropertyPixels(mozilla::dom::Element&, nsIAtom&, int) /home/worker/workspace/build/src/editor/libeditor/CSSEditUtils.cpp:473
    #11 0x7f98868d4c0e in SetAnonymousElementPosition /home/worker/workspace/build/src/editor/libeditor/HTMLAnonymousNodeEditor.cpp:571:18
    #12 0x7f98868d4c0e in mozilla::HTMLEditor::SetAllResizersPosition() /home/worker/workspace/build/src/editor/libeditor/HTMLEditorObjectResizer.cpp:241
    #13 0x7f98868d61d6 in mozilla::HTMLEditor::ShowResizersInner(nsIDOMElement*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditorObjectResizer.cpp:338:8
    #14 0x7f98867f845e in ShowResizers /home/worker/workspace/build/src/editor/libeditor/HTMLEditorObjectResizer.cpp:283:17
    #15 0x7f98867f845e in mozilla::HTMLEditor::CheckSelectionStateForAnonymousButtons(nsISelection*) /home/worker/workspace/build/src/editor/libeditor/HTMLAnonymousNodeEditor.cpp:455
    #16 0x7f98868abe9e in mozilla::HTMLEditor::EndUpdateViewBatch() /home/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:4836:10
    #17 0x7f98867aa740 in mozilla::EditorBase::EndPlaceHolderTransaction() /home/worker/workspace/build/src/editor/libeditor/EditorBase.cpp:1018:7
    #18 0x7f988689d2e0 in ~AutoPlaceHolderBatch /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/EditorUtils.h:169:16
    #19 0x7f988689d2e0 in mozilla::HTMLEditor::Align(nsAString const&) /home/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:2289
    #20 0x7f988696f942 in nsAlignCommand::SetState(nsIEditor*, nsString&) /home/worker/workspace/build/src/editor/composer/nsComposerCommands.cpp:962:22
Flags: in-testsuite?
Crash Signature: [@ nsIContent::GetBaseURIForStyleAttr const ]
Priority: -- → P1
Crash Signature: [@ nsIContent::GetBaseURIForStyleAttr const ] → [@ nsIContent::GetBaseURIForStyleAttr const ] [@ nsIContent::GetBaseURIForStyleAttr ]
This doesn't occurs when enabling Stylo.
I got this crash [1] when trying to load the attachment.

[1] https://crash-stats.mozilla.com/report/index/cdf6f635-20de-4257-82e2-6a5140170726
Crash Signature: [@ nsIContent::GetBaseURIForStyleAttr const ] [@ nsIContent::GetBaseURIForStyleAttr ] → [@ nsIContent::GetBaseURIForStyleAttr const ] [@ nsIContent::GetBaseURIForStyleAttr ] [@ libxul.so@0x1201e81 | nsDOMCSSAttributeDeclaration::GetCSSParsingEnvironment ]
Component: Editor → CSS Parsing and Computation
Summary: Crash [@nsIContent::GetBaseURIForStyleAttr()] in /home/worker/workspace/build/src/dom/base/FragmentOrElement.cpp:465 → Stylo: Crash [@nsIContent::GetBaseURIForStyleAttr()] in /home/worker/workspace/build/src/dom/base/FragmentOrElement.cpp:465
See Also: → 1384526
Priority: P1 → --
Priority: -- → P1
So this happens when we want to mutate style of an svg:use element, but that element haven't yet created the anonymous content (the cloned content).

Then this is the question: how can we have something in the anonymous subtree of svg:use element when we haven't create it? It seems something inserts a <span> into that svg:use somehow, presumably by editor.

I'm not quite familiar with editor, but I guess it should really avoid inserting anything into svg:use's anonymous subtree.

A quick and dirty fix for this, though, would be simply null-checking returned value from useElement->GetContentURLData(). When the content isn't cloned into the element, it doesn't really make sense to use the content url data there.

It would still be good to avoid inserting random element into svg:use's anonymous subtree, though.

masayuki, do you have idea how should we avoid inserting element into svg:use's anonymous subtree?
Flags: needinfo?(masayuki)
I can do the quick fix for this. It doesn't harm anyway.
Assignee: nobody → xidorn+moz
Perhaps, it's a resizer UI or its info.
https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla10HTMLEditor22CreateAnonymousElementEP7nsIAtomP10nsIDOMNodeRK9nsAStringb&redirect=false

Although, I feel it's buggy HTMLEditor to touch SVG <use> element's subtree for that...
Flags: needinfo?(masayuki)
Comment on attachment 8892711 [details]
Bug 1383780 - Null-check return value of SVGUseElement::GetContentURLData before returning.

https://reviewboard.mozilla.org/r/163704/#review169108
Attachment #8892711 - Flags: review?(bzbarsky) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5dd6f04aea59
Null-check return value of SVGUseElement::GetContentURLData before returning. r=bz
Backed out for failing crashtests on stylo:

https://hg.mozilla.org/integration/autoland/rev/9ba3d16a975bdfa01508a969ba5a4f1841cb6463

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5dd6f04aea595f0225f2bb105a34fd5d533af983&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=120266970&repo=autoland
[task 2017-08-02T05:57:27.421099Z] 05:57:27     INFO - REFTEST TEST-START | file:///home/worker/workspace/build/tests/reftest/tests/dom/base/crashtests/1383780.html
[task 2017-08-02T05:57:27.422311Z] 05:57:27     INFO - REFTEST TEST-LOAD | file:///home/worker/workspace/build/tests/reftest/tests/dom/base/crashtests/1383780.html | 276 / 3253 (8%)
[task 2017-08-02T05:57:27.426780Z] 05:57:27    ERROR - thread '<unnamed>' panicked at 'called `Option::unwrap()` on a `None` value', /checkout/src/libcore/option.rs:335
[task 2017-08-02T05:57:27.605248Z] 05:57:27     INFO - stack backtrace:
[task 2017-08-02T05:57:27.606581Z] 05:57:27     INFO -    0: std::sys::imp::backtrace::tracing::imp::unwind_backtrace
[task 2017-08-02T05:57:27.607634Z] 05:57:27     INFO -    1: std::panicking::default_hook::{{closure}}
[task 2017-08-02T05:57:27.608596Z] 05:57:27     INFO -    2: std::panicking::default_hook
[task 2017-08-02T05:57:27.611375Z] 05:57:27     INFO -    3: std::panicking::rust_panic_with_hook
[task 2017-08-02T05:57:27.612817Z] 05:57:27     INFO -    4: std::panicking::begin_panic
[task 2017-08-02T05:57:27.614518Z] 05:57:27     INFO -    5: std::panicking::begin_panic_fmt
[task 2017-08-02T05:57:27.617570Z] 05:57:27     INFO -    6: core::panicking::panic_fmt
[task 2017-08-02T05:57:27.618453Z] 05:57:27     INFO -    7: core::panicking::panic
[task 2017-08-02T05:57:27.619901Z] 05:57:27     INFO -    8: Servo_TraverseSubtree
[task 2017-08-02T05:57:27.621526Z] 05:57:27     INFO -    9: _ZN7mozilla13ServoStyleSet25PrepareAndTraverseSubtreeEPKNS_3dom7ElementENS_19ServoTraversalFlagsE
[task 2017-08-02T05:57:27.622757Z] 05:57:27     INFO -   10: _ZN7mozilla10HTMLEditor22CreateAnonymousElementEP7nsIAtomP10nsIDOMNodeRK9nsAStringb
[task 2017-08-02T05:57:27.624262Z] 05:57:27     INFO -   11: _ZN7mozilla10HTMLEditor13CreateResizerEsP10nsIDOMNode
[task 2017-08-02T05:57:27.625766Z] 05:57:27     INFO -   12: _ZN7mozilla10HTMLEditor17ShowResizersInnerEP13nsIDOMElement
[task 2017-08-02T05:57:27.627242Z] 05:57:27     INFO -   13: _ZN7mozilla10HTMLEditor12ShowResizersEP13nsIDOMElement
[task 2017-08-02T05:57:27.628730Z] 05:57:27     INFO -   14: _ZN7mozilla10HTMLEditor38CheckSelectionStateForAnonymousButtonsEP12nsISelection
[task 2017-08-02T05:57:27.631834Z] 05:57:27     INFO -   15: _ZN7mozilla10HTMLEditor18EndUpdateViewBatchEv
[task 2017-08-02T05:57:27.632629Z] 05:57:27     INFO -   16: _ZN7mozilla10EditorBase25EndPlaceHolderTransactionEv
[task 2017-08-02T05:57:27.633399Z] 05:57:27     INFO -   17: _ZN7mozilla10HTMLEditor5AlignERK9nsAString
[task 2017-08-02T05:57:27.634399Z] 05:57:27     INFO -   18: _ZN14nsAlignCommand8SetStateEP9nsIEditorR8nsString
[task 2017-08-02T05:57:27.635898Z] 05:57:27     INFO -   19: _ZN19nsMultiStateCommand15DoCommandParamsEPKcP16nsICommandParamsP11nsISupports
[task 2017-08-02T05:57:27.637410Z] 05:57:27     INFO -   20: _ZN24nsControllerCommandTable15DoCommandParamsEPKcP16nsICommandParamsP11nsISupports
[task 2017-08-02T05:57:27.638534Z] 05:57:27     INFO -   21: _ZN23nsBaseCommandController19DoCommandWithParamsEPKcP16nsICommandParams
[task 2017-08-02T05:57:27.640020Z] 05:57:27     INFO -   22: _ZN16nsCommandManager9DoCommandEPKcP16nsICommandParamsP18mozIDOMWindowProxy
[task 2017-08-02T05:57:27.641468Z] 05:57:27     INFO -   23: _ZN14nsHTMLDocument11ExecCommandERK9nsAStringbS2_R12nsIPrincipalRN7mozilla11ErrorResultE
[task 2017-08-02T05:57:27.642948Z] 05:57:27     INFO -   24: _ZN7mozilla3dom19HTMLDocumentBindingL11execCommandEP9JSContextN2JS6HandleIP8JSObjectEEP14nsHTMLDocumentRK19JSJitMethodCallArgs
[task 2017-08-02T05:57:27.644073Z] 05:57:27     INFO -   25: _ZN7mozilla3dom20GenericBindingMethodEP9JSContextjPN2JS5ValueE
[task 2017-08-02T05:57:27.645502Z] 05:57:27     INFO -   26: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
[task 2017-08-02T05:57:27.646971Z] 05:57:27     INFO -   27: _ZL9InterpretP9JSContextRN2js8RunStateE
[task 2017-08-02T05:57:27.648376Z] 05:57:27     INFO -   28: _ZN2js9RunScriptEP9JSContextRNS_8RunStateE
[task 2017-08-02T05:57:27.649475Z] 05:57:27     INFO -   29: _ZN2js7ExecuteEP9JSContextN2JS6HandleIP8JSScriptEER8JSObjectPNS2_5ValueE
[task 2017-08-02T05:57:27.650916Z] 05:57:27     INFO -   30: _ZL13ExecuteScriptP9JSContextRN2JS16AutoObjectVectorENS1_6HandleIP8JSScriptEEPNS1_5ValueE
[task 2017-08-02T05:57:27.652406Z] 05:57:27     INFO -   31: _ZN9nsJSUtils16ExecutionContext14CompileAndExecERN2JS14CompileOptionsERNS1_18SourceBufferHolderENS1_13MutableHandleIP8JSScriptEE
[task 2017-08-02T05:57:27.653272Z] 05:57:27     INFO -   32: _ZN7mozilla3dom12ScriptLoader14EvaluateScriptEPNS0_17ScriptLoadRequestE.part.312
[task 2017-08-02T05:57:27.654113Z] 05:57:27     INFO -   33: _ZN7mozilla3dom12ScriptLoader14ProcessRequestEPNS0_17ScriptLoadRequestE
[task 2017-08-02T05:57:27.654873Z] 05:57:27     INFO -   34: _ZN7mozilla3dom12ScriptLoader20ProcessScriptElementEP16nsIScriptElement
[task 2017-08-02T05:57:27.655662Z] 05:57:27     INFO -   35: _ZN7mozilla3dom13ScriptElement18MaybeProcessScriptEv
[task 2017-08-02T05:57:27.656445Z] 05:57:27     INFO -   36: _ZN21nsHtml5TreeOpExecutor9RunScriptEP10nsIContent
[task 2017-08-02T05:57:27.657245Z] 05:57:27     INFO -   37: _ZN21nsHtml5TreeOpExecutor12RunFlushLoopEv
[task 2017-08-02T05:57:27.658001Z] 05:57:27     INFO -   38: _ZN22nsHtml5ExecutorFlusher3RunEv
[task 2017-08-02T05:57:27.658771Z] 05:57:27     INFO -   39: _ZN8nsThread16ProcessNextEventEbPb.part.142
[task 2017-08-02T05:57:27.659553Z] 05:57:27     INFO -   40: _Z19NS_ProcessNextEventP9nsIThreadb
[task 2017-08-02T05:57:27.660320Z] 05:57:27     INFO -   41: _ZN7mozilla3ipc11MessagePump3RunEPN4base11MessagePump8DelegateE
[task 2017-08-02T05:57:27.661102Z] 05:57:27     INFO -   42: _ZN11MessageLoop3RunEv
[task 2017-08-02T05:57:27.661843Z] 05:57:27     INFO -   43: _ZN14nsBaseAppShell3RunEv
[task 2017-08-02T05:57:27.662601Z] 05:57:27     INFO -   44: _ZN12nsAppStartup3RunEv
[task 2017-08-02T05:57:27.663422Z] 05:57:27     INFO -   45: _ZN7XREMain11XRE_mainRunEv
[task 2017-08-02T05:57:27.664184Z] 05:57:27     INFO -   46: _ZN7XREMain8XRE_mainEiPPcRKN7mozilla15BootstrapConfigE
[task 2017-08-02T05:57:27.664944Z] 05:57:27     INFO -   47: _Z8XRE_mainiPPcRKN7mozilla15BootstrapConfigE
[task 2017-08-02T05:57:27.665702Z] 05:57:27     INFO -   48: _ZL7do_mainiPPcS0_
[task 2017-08-02T05:57:27.666434Z] 05:57:27     INFO -   49: main
[task 2017-08-02T05:57:27.667208Z] 05:57:27     INFO -   50: __libc_start_main
[task 2017-08-02T05:57:27.667961Z] 05:57:27     INFO -   51: <unknown>
[task 2017-08-02T05:57:27.668710Z] 05:57:27     INFO - Redirecting call to abort() to mozalloc_abort
[task 2017-08-02T05:57:27.669504Z] 05:57:27     INFO - ExceptionHandler::GenerateDump cloned child 1092
[task 2017-08-02T05:57:27.670279Z] 05:57:27     INFO - ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2017-08-02T05:57:27.671043Z] 05:57:27     INFO - ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2017-08-02T05:57:27.801066Z] 05:57:27    ERROR - TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/base/crashtests/1383780.html | application terminated with exit code 11
Flags: needinfo?(xidorn+moz)
Turns out the crash in comment 0 isn't the only crash revealed by this testcase :/
I don't know why but the testcase now crashes with
> Assertion failure: MayTraverseFrom(const_cast<Element*>(aRoot)), at /layout/style/ServoStyleSet.cpp:275

I believe I didn't see this when I submitted that patch, so presumably some changes in-between touched this.
So the MayTraverseFrom assertion fails because the parent of that element is the svg:use element, which doesn't have ServoData yet.

I'm not sure what's going on, then. I guess one way to fix both issue is to avoid creating resizer in this case. (We may still want to preserve the null-check in the original patch, though.)
Flags: needinfo?(xidorn+moz)
Comment on attachment 8893209 [details]
Bug 1383780 - Avoid adding anonymous editor element into non-HTML element.

https://reviewboard.mozilla.org/r/164242/#review169578

::: commit-message-8f4d1:1
(Diff revision 1)
> +Bug 1383780 - Avoid adding anonymous editor element into non-HTML element. r?masayuki
> +
> +MozReview-Commit-ID: J5kCUzA7K90

Please explain the "reason" in this commit message

::: editor/libeditor/HTMLAnonymousNodeEditor.cpp:179
(Diff revision 1)
> +  // Don't put anonymous editor element into non-HTML element.
> +  if (!parentContent->IsHTMLElement()) {

And also here. The reason is important (especially it's for svg::use since somebody wants to create anonymous UI for other non-HTML elements).
Attachment #8893209 - Flags: review?(masayuki) → review+
Comment on attachment 8893209 [details]
Bug 1383780 - Avoid adding anonymous editor element into non-HTML element.

https://reviewboard.mozilla.org/r/164242/#review169578

> And also here. The reason is important (especially it's for svg::use since somebody wants to create anonymous UI for other non-HTML elements).

The problem isn't that somebody wants to create anonymous UI for non-HTML element. It is somebody wants to create anonymous UI for an element inside a non-HTML parent, and thus the anonymous element is inserted into that parent.
Comment on attachment 8893209 [details]
Bug 1383780 - Avoid adding anonymous editor element into non-HTML element.

Are you fine with that comment?
Attachment #8893209 - Flags: review+ → review?(masayuki)
Comment on attachment 8893209 [details]
Bug 1383780 - Avoid adding anonymous editor element into non-HTML element.

Fine, thanks. But I still would like you to add some comments to the commit message too since writing the reason of the change is our generic rule.
Attachment #8893209 - Flags: review?(masayuki) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c64ead4af0a
Avoid adding anonymous editor element into non-HTML element. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/192f97d9e355
Null-check return value of SVGUseElement::GetContentURLData before returning. r=bz
https://hg.mozilla.org/mozilla-central/rev/8c64ead4af0a
https://hg.mozilla.org/mozilla-central/rev/192f97d9e355
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Xidorn, should we uplift this crash fix to Beta 56? We will be running a Stylo experiment with some unsuspecting Beta 56 users.
status-firefox55: --- → unaffected
status-firefox56: --- → ?
Flags: needinfo?(xidorn+moz)
This change has certain risk to non-stylo part, so I would be rather reluctant to uplift it. Also this is a crash from fuzz, not from a real world crash report. I think this kind of specific case should rarely appear in any sensible website. Given these, I would suggest we don't uplift it.
Flags: needinfo?(xidorn+moz)
status-firefox56 = wontfix
status-firefox56: ? → wontfix
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: