1350972 - Crash [@InvalidateAllFrames]

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Testcase found while fuzzing mozilla-central rev 20170326-cc53710589fb.

ASAN:DEADLYSIGNAL
=================================================================
==5156==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000020 (pc 0x7f364628bc2e bp 0x7ffdfa6ab030 sp 0x7ffdfa6ab000 T0)
==5156==The signal is caused by a READ memory access.
==5156==Hint: address points to the zero page.
    #0 0x7f364628bc2d in get /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:284:27
    #1 0x7f364628bc2d in operator-> /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:316
    #2 0x7f364628bc2d in NodeType /home/worker/workspace/build/src/dom/base/nsINode.h:585
    #3 0x7f364628bc2d in InvalidateAllFrames /home/worker/workspace/build/src/dom/base/nsRange.cpp:61
    #4 0x7f364628bc2d in nsRange::AutoInvalidateSelection::~AutoInvalidateSelection() /home/worker/workspace/build/src/dom/base/nsRange.cpp:3275
    #5 0x7f3646279c35 in nsRange::SelectNode(nsINode&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/nsRange.cpp:1521:1
    #6 0x7f3646279e0f in nsRange::SelectNodeJS(nsINode&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/nsRange.cpp:1494:3
    #7 0x7f3646adace6 in mozilla::dom::RangeBinding::selectNode(JSContext*, JS::Handle<JSObject*>, nsRange*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/RangeBinding.cpp:775:9
    #8 0x7f36479fbcbe in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2953:13
    #9 0x7f364d1ccdd3 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:282:15
    #10 0x7f364d1ccdd3 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:454
    #11 0x7f364d1b5618 in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:505:12
    #12 0x7f364d1b5618 in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2998
    #13 0x7f364d19b57e in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:394:12
    #14 0x7f364d1cf2e7 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:683:15
    #15 0x7f364d1cfb52 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:715:12

Comment 1

[Andrew Overholt [:overholt]]

Do you know if this is a recent regression?

Comment 2

[Jason Kratzer [:jkratzer]]

Unfortunately I don't have an automated way of determining when this bug was introduced.  This is something we're working towards but likely won't have an answer for you any time soon.

Comment 3

[John Dai[:jdai]]

(In reply to Andrew Overholt [:overholt] from comment #1)
> Do you know if this is a recent regression?

It seems not a recent regression, it shows 2015-09-14 on the mozregression in Firefox 43.

Regression window:
 9:50.85 INFO: Narrowed nightly regression window from [2015-09-07, 2015-09-20] (13 days) to [2015-09-07, 2015-09-14] (7 days) (~2 steps left)
 9:50.85 INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=df817a53e24bbacd62347cb9873d86039448b666&tochange=9ed17db42e3e46f1c712e4dffd62d54e915e0fac


Regressed by: Bug 571294

Comment 4

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

Mystor, could you take a look? Probably not an issue in your patch but just old missing null check or something.

Comment 5

[John Dai[:jdai]]

At my first glance, aNode[1] is NULL when it calls InvalidateAllFrames.

[1] http://searchfox.org/mozilla-central/source/dom/base/nsRange.cpp#61

Comment 6

[John Dai[:jdai]]

Attachment: patch, v1

It might returns NULL when AutoInvalidateSelection dtor doesn't have commonAncestor[1][2]. Add a null check to guard it.

[1] https://searchfox.org/mozilla-central/source/dom/base/nsRange.cpp#3273
[2] https://searchfox.org/mozilla-central/source/dom/base/nsRange.cpp#148

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b55850033e3e9be75a70b90edd3d09e7f443cd66&group_state=expanded&filter-tier=1

Comment 7

[John Dai[:jdai]]

Attachment: patch, v2

- Add crashtest asserts expect numbers.
- Carry r+.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd556d161d95b591f806da146726a283e359d4bd&filter-tier=1&group_state=expanded

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4117dddd0a72
Avoid crash when AutoInvalidateSelection dtor doesn't have commonAncestor. r=smaug

Comment 9

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/4117dddd0a72

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 11

[John Dai[:jdai]]

Attachment: (Aurora) patch, final

Approval Request Comment
[Feature/Bug causing the regression]: Range and selection
[User impact if declined]: user use Range.selectNode() can crash
[Is this code covered by automated tests?]: Yes, it's located at dom/html/crashtests/1350972.html 
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: This change mainly added a null check, it can make our code more robust.
[String changes made/needed]: none

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8857749 [details] [diff] [review]
(Aurora) patch, final

Fix a crash. Aurora54+.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/87db9cca835a