Closed
Bug 1341657
Opened 8 years ago
Closed 8 years ago
Crash [@nsDocShell::InternalLoad]
Categories
(Core :: DOM: Navigation, defect, P2)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: jkratzer, Assigned: ehsan.akhgari)
References
(Blocks 1 open bug)
Details
(Keywords: crash, csectype-nullptr, testcase)
Crash Data
Attachments
(3 files)
|
520 bytes,
text/html
|
Details | |
|
4.50 KB,
patch
|
smaug
:
review+
jcristau
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
|
4.47 KB,
patch
|
Details | Diff | Splinter Review |
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?
|
Reporter | |
Updated•8 years ago
|
Component: DOM → Document Navigation
|
||
Comment 2•8 years ago
|
||
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
|
||
Comment 3•8 years ago
|
||
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]
|
Assignee | |
Comment 4•8 years ago
|
||
(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)
|
|
Assignee | |
Comment 5•8 years ago
|
||
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>
|
|
Assignee | |
Comment 6•8 years ago
|
||
Attachment #8841127 -
Flags: review?(bugs)
|
|
Assignee | |
Comment 7•8 years ago
|
||
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?
|
||
Updated•8 years ago
|
Attachment #8841127 -
Flags: review?(bugs) → review+
|
||
Comment 8•8 years ago
|
||
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+
|
||
Comment 9•8 years ago
|
||
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)
|
||
Comment 10•8 years ago
|
||
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
|
|
Assignee | |
Comment 11•8 years ago
|
||
(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)
|
|
Assignee | |
Updated•8 years ago
|
|
|
Assignee | |
Comment 12•8 years ago
|
||
|
|
Assignee | |
Comment 13•8 years ago
|
||
|
|
||
Comment 14•8 years ago
|
||
(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)
|
|
Assignee | |
Comment 15•8 years ago
|
||
(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)
|
||
Updated•8 years ago
|
Flags: needinfo?(cbook)
|
||
Comment 16•8 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
|
|
||
Comment 17•8 years ago
|
||
| bugherder uplift | ||
Flags: in-testsuite? → in-testsuite+
|
|
||
Comment 18•8 years ago
|
||
| bugherder uplift | ||
status-firefox-esr52:
--- → fixed
|
|
||
Comment 19•8 years ago
|
||
| bugherder uplift | ||
You need to log in
before you can comment on or make changes to this bug.
Description
•