Closed Bug 1341657 Opened 3 years ago Closed 3 years ago

Crash [@nsDocShell::InternalLoad]

Categories

(Core :: Document Navigation, defect, P2, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: jkratzer, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

(Keywords: crash, csectype-nullptr, testcase)

Crash Data

Attachments

(3 files)

Attached file index.html
Testcase found by fuzzing on mozilla-central rev 20170216-a9ec72f82299.

==14997==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000020 (pc 0x7f5b4aa61ae8 bp 0x7ffc394456e0 sp 0x7ffc39444940 T0)
    #0 0x7f5b4aa61ae7 in get /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:283:27
    #1 0x7f5b4aa61ae7 in operator-> /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:315
    #2 0x7f5b4aa61ae7 in IsHTMLElement /home/worker/workspace/build/src/obj-firefox/dist/include/nsIContent.h:275
    #3 0x7f5b4aa61ae7 in nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, nsAString_internal const&, char const*, nsAString_internal const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString_internal const&, nsIDocShell*, nsIURI*, bool, nsIDocShell**, nsIRequest**) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:9891
    #4 0x7f5b4aa5e3b0 in nsDocShell::LoadURI(nsIURI*, nsIDocShellLoadInfo*, unsigned int, bool) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:1564:10
    #5 0x7f5b4446eac5 in mozilla::dom::Location::SetURI(nsIURI*, bool) /home/worker/workspace/build/src/dom/base/Location.cpp:288:12
    #6 0x7f5b44471de2 in mozilla::dom::Location::SetHrefWithBase(nsAString_internal const&, nsIURI*, bool) /home/worker/workspace/build/src/dom/base/Location.cpp:550:12
    #7 0x7f5b4447159f in SetHrefWithContext /home/worker/workspace/build/src/dom/base/Location.cpp:503:10
    #8 0x7f5b4447159f in mozilla::dom::Location::SetHref(nsAString_internal const&) /home/worker/workspace/build/src/dom/base/Location.cpp:472
    #9 0x7f5b44c1d1f0 in SetHref /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Location.h:89:14
    #10 0x7f5b44c1d1f0 in mozilla::dom::LocationBinding::set_href(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Location*, JSJitSetterCallArgs) /home/worker/workspace/build/src/obj-firefox/dom/bindings/LocationBinding.cpp:96
    #11 0x7f5b44c1c8eb in mozilla::dom::LocationBinding::genericCrossOriginSetter(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/obj-firefox/dom/bindings/LocationBinding.cpp:968:8
    #12 0x7f5b4ba4be24 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:281:15
    #13 0x7f5b4ba4be24 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:463
    #14 0x7f5b4ba4da24 in InternalCall /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:508:12
    #15 0x7f5b4ba4da24 in Call /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:527
Flags: in-testsuite?
Component: DOM → Document Navigation
Bug 1148044 missed a null check.
Blocks: 1148044
Ehsan, you worked on bug 1148044. Andrew McCreight said there were 38 crashes in the last week with this signature so not the most common crash :)
Flags: needinfo?(ehsan)
Priority: -- → P2
It is useful to add the crash signature to these bugs, in case we're seeing a bunch of these crashes in the wild, so people can more easily find this bug. And also it makes it easier to see how common this might be in the wild.
Crash Signature: [@nsDocShell::InternalLoad]
(In reply to Olli Pettay [:smaug] (pto-ish for couple of days) from comment #1)
> Bug 1148044 missed a null check.

In my defense, this bogus assertion is to blame: <https://hg.mozilla.org/mozilla-central/rev/ad97d9bfc686#l1.11>
Assignee: nobody → ehsan
Flags: needinfo?(ehsan)
Comment on attachment 8839964 [details]
index.html

><html>
>    <head>
>        <script>
>            o1 = document.createElement("script");
>            o2 = document.implementation.createDocument('', '', null);
>            o3 = document.createElement("iframe");
>            document.documentElement.appendChild(o3);
>            o4 = o3.contentWindow;
>            o5 = document.createTextNode('o2.adoptNode(o3); try { o4.location = "" } catch(e) {}');

Here, adoptNode destroys the iframe, nulling out mFrameElement:

#3  0x00007fe37d4e39d1 in nsPIDOMWindowOuter::SetFrameElementInternal(mozilla::dom::Element*) (this=0x7fe35531b020, aFrameElement=0x0)
    at /home/ehsan/moz/src3/dom/base/nsGlobalWindow.cpp:3975
#4  0x00007fe37d6e8bfc in nsFrameLoader::StartDestroy() (this=0x7fe3559c6700) at /home/ehsan/moz/src3/dom/base/nsFrameLoader.cpp:2022
#5  0x00007fe37d6e8740 in nsFrameLoader::Destroy() (this=0x7fe3559c6700) at /home/ehsan/moz/src3/dom/base/nsFrameLoader.cpp:1928
#6  0x00007fe37e9eb250 in nsGenericHTMLFrameElement::UnbindFromTree(bool, bool) (this=0x7fe3558ca3d0, aDeep=true, aNullParent=true)
    at /home/ehsan/moz/src3/dom/html/nsGenericHTMLFrameElement.cpp:303
#7  0x00007fe37d73fe8c in nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) (this=0x7fe3568ea1c0, aIndex=1, aNotify=true, aKid=0x7fe3558ca3d0, aChildArray=...) at /home/ehsan/moz/src3/dom/base/nsINode.cpp:1922
#8  0x00007fe37d5a5e5b in mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool) (this=0x7fe3568ea1c0, aIndex=1, aNotify=true)
    at /home/ehsan/moz/src3/dom/base/FragmentOrElement.cpp:1170
#9  0x00007fe37d6b32df in nsIDocument::AdoptNode(nsINode&, mozilla::ErrorResult&) (this=0x7fe3568e9000, aAdoptedNode=..., rv=...)
    at /home/ehsan/moz/src3/dom/base/nsDocument.cpp:7492
#10 0x00007fe37e273806 in mozilla::dom::DocumentBinding::adoptNode(JSContext*, JS::Handle<JSObject*>, nsIDocument*, JSJitMethodCallArgs const&) (cx=0x7fe36eadd000, obj=(JSObject * const) 0x7fe367b13100 [object XMLDocument], self=0x7fe3568e9000, args=...) at /home/ehsan/moz/src3/obj-ff-dbg/dom/bindings/DocumentBinding.cpp:1366

So we should just deal with the null pointer here.

>            o1.appendChild(o5);
>            document.documentElement.appendChild(o1);
>        </script>
>    </head>
></html>
Comment on attachment 8841127 [details] [diff] [review]
Properly deal with not having a frame element in nsDocShell::InternalLoad()

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1148044
[User impact if declined]: Crash.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet, but it's super safe.
[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?]: Not at all.
[Why is the change risky/not risky?]: This is just a null check.  Technically it slightly changes our behavior, but the behavior implemented in bug 1148044 isn't exposed to the Web so the change won't affect anything.
[String changes made/needed]: None.
Attachment #8841127 - Flags: approval-mozilla-beta?
Attachment #8841127 - Flags: approval-mozilla-aurora?
Attachment #8841127 - Flags: review?(bugs) → review+
Comment on attachment 8841127 [details] [diff] [review]
Properly deal with not having a frame element in nsDocShell::InternalLoad()

crash fix for aurora53 and beta52
Attachment #8841127 - Flags: approval-mozilla-beta?
Attachment #8841127 - Flags: approval-mozilla-beta+
Attachment #8841127 - Flags: approval-mozilla-aurora?
Attachment #8841127 - Flags: approval-mozilla-aurora+
does not apply cleanly on aurora and needs rebasing

renamed 1341657 -> Bug-1341657---Properly-deal-with-not-having-a-fram.patch
applying Bug-1341657---Properly-deal-with-not-having-a-fram.patch
patching file docshell/base/crashtests/crashtests.list
Hunk #1 FAILED at 8
1 out of 1 hunks FAILED -- saving rejects to file docshell/base/crashtests/crashtests.list.rej
patching file docshell/base/nsDocShell.cpp
Hunk #1 FAILED at 9886
1 out of 2 hunks FAILED -- saving rejects to file docshell/base/nsDocShell.cpp.rej
Flags: needinfo?(ehsan)
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35a06336b3be
Properly deal with not having a frame element in nsDocShell::InternalLoad(); r=smaug
(In reply to Carsten Book [:Tomcat] from comment #9)
> does not apply cleanly on aurora and needs rebasing
> 
> renamed 1341657 -> Bug-1341657---Properly-deal-with-not-having-a-fram.patch
> applying Bug-1341657---Properly-deal-with-not-having-a-fram.patch
> patching file docshell/base/crashtests/crashtests.list
> Hunk #1 FAILED at 8
> 1 out of 1 hunks FAILED -- saving rejects to file
> docshell/base/crashtests/crashtests.list.rej
> patching file docshell/base/nsDocShell.cpp
> Hunk #1 FAILED at 9886
> 1 out of 2 hunks FAILED -- saving rejects to file
> docshell/base/nsDocShell.cpp.rej

When trying to push to beta, I got: "mozilla-beta is CLOSED! Reason: Bug 1318918 - Merge day"
Flags: needinfo?(ehsan) → needinfo?(cbook)
(In reply to :Ehsan Akhgari from comment #11)
> (In reply to Carsten Book [:Tomcat] from comment #9)
> > does not apply cleanly on aurora and needs rebasing
> > 
> > renamed 1341657 -> Bug-1341657---Properly-deal-with-not-having-a-fram.patch
> > applying Bug-1341657---Properly-deal-with-not-having-a-fram.patch
> > patching file docshell/base/crashtests/crashtests.list
> > Hunk #1 FAILED at 8
> > 1 out of 1 hunks FAILED -- saving rejects to file
> > docshell/base/crashtests/crashtests.list.rej
> > patching file docshell/base/nsDocShell.cpp
> > Hunk #1 FAILED at 9886
> > 1 out of 2 hunks FAILED -- saving rejects to file
> > docshell/base/nsDocShell.cpp.rej
> 
> When trying to push to beta, I got: "mozilla-beta is CLOSED! Reason: Bug
> 1318918 - Merge day"

yeah releng is merging beta to release so would need to land on release too
Flags: needinfo?(cbook)
(In reply to Carsten Book [:Tomcat] from comment #14)
> (In reply to :Ehsan Akhgari from comment #11)
> > (In reply to Carsten Book [:Tomcat] from comment #9)
> > > does not apply cleanly on aurora and needs rebasing
> > > 
> > > renamed 1341657 -> Bug-1341657---Properly-deal-with-not-having-a-fram.patch
> > > applying Bug-1341657---Properly-deal-with-not-having-a-fram.patch
> > > patching file docshell/base/crashtests/crashtests.list
> > > Hunk #1 FAILED at 8
> > > 1 out of 1 hunks FAILED -- saving rejects to file
> > > docshell/base/crashtests/crashtests.list.rej
> > > patching file docshell/base/nsDocShell.cpp
> > > Hunk #1 FAILED at 9886
> > > 1 out of 2 hunks FAILED -- saving rejects to file
> > > docshell/base/nsDocShell.cpp.rej
> > 
> > When trying to push to beta, I got: "mozilla-beta is CLOSED! Reason: Bug
> > 1318918 - Merge day"
> 
> yeah releng is merging beta to release so would need to land on release too

I don't know how to do that.  I ni?ed you hoping you would help me with that.
Flags: needinfo?(cbook)
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/35a06336b3be
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1342184
You need to log in before you can comment on or make changes to this bug.