All users were logged out of Bugzilla on October 13th, 2018

crash near null [@nsNullPrincipal::CreateWithInheritedAttributes]

RESOLVED FIXED in Firefox 52



2 years ago
a year ago


(Reporter: tsmith, Assigned: baku)


(Blocks: 2 bugs, {crash, csectype-nullptr, testcase})

crash, csectype-nullptr, testcase
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)



(2 attachments, 1 obsolete attachment)



2 years ago
Created attachment 8826997 [details]

==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)

Comment 2

2 years ago
Hi Baku, could you help to look into this bug?
Flags: needinfo?(amarchesini)


2 years ago
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)

Comment 3

2 years ago
Created attachment 8837057 [details] [diff] [review]

It seems to me that we could have a null principal here (where null means a null pointer).
Attachment #8837057 - Flags: review?(bugs)


2 years ago
Blocks: 1022229

Comment 4

2 years ago
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-

Comment 5

2 years ago
Created attachment 8837147 [details] [diff] [review]
Attachment #8837057 - Attachment is obsolete: true
Attachment #8837147 - Flags: review?(bugs)


2 years ago
Attachment #8837147 - Flags: review?(bugs) → review+

Comment 6

2 years ago
Please add a testcase too. I guess the attached testcase as a crashtest would work.

Comment 7

2 years ago
Pushed by
nsNullPrincipal should be created using OriginAttributes of the docShell in case the principal is null, r=smaug

Comment 8

2 years ago
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
status-firefox52: --- → affected
Flags: needinfo?(amarchesini)
Flags: in-testsuite+

Comment 10

2 years ago
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+

Comment 12

2 years ago
status-firefox53: affected → fixed

Comment 13

2 years ago
status-firefox52: affected → fixed

Comment 14

2 years ago
What landed contains the test. The crashtast is already in m-i/m-a/m-b.
Flags: needinfo?(amarchesini)

Comment 15

2 years ago
status-firefox-esr52: --- → fixed
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)

Comment 17

a year ago
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.