Closed Bug 1331295 Opened 7 years ago Closed 6 years ago

crash near null [@nsNullPrincipal::CreateWithInheritedAttributes]


(Core :: Security: CAPS, defect)

Not set



Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed


(Reporter: tsmith, Assigned: baku)


(Blocks 2 open bugs)


(Keywords: crash, csectype-nullptr, testcase)


(2 files, 1 obsolete file)

Attached file test_case.html
==24308==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f0bbda83399 bp 0x7ffc304f2510 sp 0x7ffc304f2460 T0)
    #0 0x7f0bbda83398 in nsNullPrincipal::CreateWithInheritedAttributes(nsIPrincipal*) /home/worker/workspace/build/src/caps/nsNullPrincipal.cpp:43:32
    #1 0x7f0bc3858e5f in nsDocShell::CreateAboutBlankContentViewer(nsIPrincipal*, nsIURI*, bool, bool) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:8049:19
    #2 0x7f0bc37f7a59 in nsDocShell::EnsureContentViewer() /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:7941:17
    #3 0x7f0bc37f408c in nsDocShell::GetInterface(nsID const&, void**) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:971:14
    #4 0x7f0bbbfa2142 in nsGetInterface::operator()(nsID const&, void**) const /home/worker/workspace/build/src/xpcom/glue/nsIInterfaceRequestorUtils.cpp:18:16
    #5 0x7f0bbbf93f15 in nsCOMPtr_base::assign_from_helper(nsCOMPtr_helper const&, nsID const&) /home/worker/workspace/build/src/xpcom/glue/nsCOMPtr.cpp:117:7
    #6 0x7f0bbd9d934e in nsCOMPtr /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:574:5
    #7 0x7f0bbd9d934e in nsDocLoader::DocLoaderIsEmpty(bool) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:667
    #8 0x7f0bbd9dba34 in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:612:5
    #9 0x7f0bbd9dc5ec in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /home/worker/workspace/build/src/uriloader/base/nsDocLoader.cpp:468:14
    #10 0x7f0bbc1187ab in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /home/worker/workspace/build/src/netwerk/base/nsLoadGroup.cpp:633:18
    #11 0x7f0bbc9ddf26 in mozilla::net::nsHttpChannel::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /home/worker/workspace/build/src/netwerk/protocol/http/nsHttpChannel.cpp:6877:9
    #12 0x7f0bbc1119d1 in nsInputStreamPump::OnStateStop() /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:714:9
    #13 0x7f0bbc10f971 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) /home/worker/workspace/build/src/netwerk/base/nsInputStreamPump.cpp:433:25
    #14 0x7f0bbbee6dad in nsInputStreamReadyEvent::Run() /home/worker/workspace/build/src/xpcom/io/nsStreamUtils.cpp:95:9
    #15 0x7f0bbbf3042b in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1240:7
    #16 0x7f0bbbfb42fc in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/glue/nsThreadUtils.cpp:390:10
    #17 0x7f0bbcd6c2df in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #18 0x7f0bbccda2a8 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/
    #19 0x7f0bbccda2a8 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/
    #20 0x7f0bbccda2a8 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/
    #21 0x7f0bc2181e2f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #22 0x7f0bc42173f1 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:19
    #23 0x7f0bc43ae04c in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4494:10
    #24 0x7f0bc43af51e in XREMain::XRE_main(int, char**, mozilla::XREAppData const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4623:8
    #25 0x7f0bc43b041c in XRE_main /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4714:16
    #26 0x4df90c in do_main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:319:10
    #27 0x4df90c in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:452
    #28 0x7f0bd72a782f in __libc_start_main /build/glibc-t3gR2i/glibc-2.23/csu/../csu/libc-start.c:291
    #29 0x41ba38 in _start (/home/user/workspace/browsers/firefox_cnt/firefox+0x41ba38)
Confirmed, definitely crashes in 54.0a1 (2017-02-13) (64-bit)
Hi Baku, could you help to look into this bug?
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attached patch null.patch (obsolete) — Splinter Review
It seems to me that we could have a null principal here (where null means a null pointer).
Attachment #8837057 - Flags: review?(bugs)
Comment on attachment 8837057 [details] [diff] [review]

So we used to create null principal here before bug 1022229 (and not requiring aPrincipal to be non-null).
I think the current setup is just a bit broken.
We should still create null principal even if there isn't aPrincipal, and inherit OA from docshell?
Attachment #8837057 - Flags: review?(bugs) → review-
Attached patch null.patchSplinter Review
Attachment #8837057 - Attachment is obsolete: true
Attachment #8837147 - Flags: review?(bugs)
Attachment #8837147 - Flags: review?(bugs) → review+
Please add a testcase too. I guess the attached testcase as a crashtest would work.
Pushed by
nsNullPrincipal should be created using OriginAttributes of the docShell in case the principal is null, r=smaug
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(amarchesini)
Flags: in-testsuite+
Comment on attachment 8837147 [details] [diff] [review]

Approval Request Comment
[Feature/Bug causing the regression]: bug 1022229
[User impact if declined]: a crash can occur
[Is this code covered by automated tests?]: there is a crashtest included in the patch
[Has the fix been verified in Nightly?]: not yet 
[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?]: no
[Why is the change risky/not risky?]: We check if the principal is null, and in case we use a different CTOR of a null principal.
[String changes made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8837147 - Flags: approval-mozilla-beta?
Attachment #8837147 - Flags: approval-mozilla-aurora?
Comment on attachment 8837147 [details] [diff] [review]

Should fix some crashes, let's take it for beta 8. 
baku do you want to uplift the tests also?
Flags: needinfo?(amarchesini)
Attachment #8837147 - Flags: approval-mozilla-beta?
Attachment #8837147 - Flags: approval-mozilla-beta+
Attachment #8837147 - Flags: approval-mozilla-aurora?
Attachment #8837147 - Flags: approval-mozilla-aurora+
What landed contains the test. The crashtast is already in m-i/m-a/m-b.
Flags: needinfo?(amarchesini)
Andrea, how important is the window.reload() at the end of this test? Is there a way to avoid that without making the test useless?

The reftest harness receives an event when the page loads, and that reload is causing us to do a bunch of stuff twice. It's preventing us from landing bug 1372565 and may be messing up some other state. Thanks!
Blocks: 1372565
Flags: needinfo?(amarchesini)
I think you can remove that window.reload() yes. That that is mainly copy-and-paste of the original crash test case.
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.