1761728 - Crash [@ RefPtr<mozilla::dom::SessionHistoryEntry>::get]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Christian Holler (:decoder)]

In experimental IPC fuzzing, we found the following crash on mozilla-central revision 4d80f4e1809a+:

=================================================================
==1701==ERROR: AddressSanitizer: SEGV on unknown address 0x0000000004d8 (pc 0x7fffe7c1833f bp 0x7fffffdc8720 sp 0x7fffffdc8700 T0)
    #0 0x7fffe7c1833f in RefPtr<mozilla::dom::SessionHistoryEntry>::get() const objdir-ff-asan/dist/include/mozilla/RefPtr.h:286:27
    #1 0x7fffe7c1833f in RefPtr<mozilla::dom::SessionHistoryEntry>::operator mozilla::dom::SessionHistoryEntry*() const & objdir-ff-asan/dist/include/mozilla/RefPtr.h:299:12
    #2 0x7fffe7c1833f in mozilla::dom::CanonicalBrowsingContext::GetActiveSessionHistoryEntry() docshell/base/CanonicalBrowsingContext.cpp:470:10
    #3 0x7fffdccbbf28 in mozilla::dom::ContentParent::RecvSynchronizeLayoutHistoryState(mozilla::dom::MaybeDiscarded<mozilla::dom::BrowsingContext> const&, nsILayoutHistoryState*) dom/ipc/ContentParent.cpp:7338:50
    #4 0x7fffdd30fc51 in mozilla::dom::PContentParent::OnMessageReceived(IPC::Message const&) objdir-ff-asan/ipc/ipdl/PContentParent.cpp:8088:57
    #5 0x7fffccf9e0d0 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message const&) ipc/glue/MessageChannel.cpp:1861:25
    #6 0x7fffccf9713a in mozilla::ipc::MessageChannel::DispatchMessage(mozilla::ipc::ActorLifecycleProxy*, IPC::Message&&) ipc/glue/MessageChannel.cpp:1783:9
    #7 0x7fffccf98f86 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::ActorLifecycleProxy*, mozilla::ipc::MessageChannel::MessageTask&) ipc/glue/MessageChannel.cpp:1488:3
    #8 0x7fffccf9b4b0 in mozilla::ipc::MessageChannel::MessageTask::Run() ipc/glue/MessageChannel.cpp:1607:14
    #9 0x7fffc9720dcb in mozilla::RunnableTask::Run() xpcom/threads/TaskController.cpp:467:16
    #10 0x7fffc967908e in mozilla::TaskController::DoExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) xpcom/threads/TaskController.cpp:775:26
    #11 0x7fffc9672259 in mozilla::TaskController::ExecuteNextTaskOnlyMainThreadInternal(mozilla::detail::BaseAutoLock<mozilla::Mutex&> const&) xpcom/threads/TaskController.cpp:611:15
    #12 0x7fffc96739e1 in mozilla::TaskController::ProcessPendingMTTask(bool) xpcom/threads/TaskController.cpp:390:36
    #13 0x7fffc96fab1f in mozilla::TaskController::InitializeInternal()::$_0::operator()() const xpcom/threads/TaskController.cpp:124:37
    #14 0x7fffc96fab1f in mozilla::detail::RunnableFunction<mozilla::TaskController::InitializeInternal()::$_0>::Run() objdir-ff-asan/dist/include/nsThreadUtils.h:531:5
    #15 0x7fffc96c10c1 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:1173:16
    #16 0x7fffc96dbb98 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/threads/nsThreadUtils.cpp:467:10
    #17 0x7fffccfac986 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:85:21
    #18 0x7fffccbeb30f in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:331:10
    [...]

We don't have a testcase mechanism yet, but this looks like an easy fix (missing a null check, I will test locally and submit a patch). Marking s-s because IPC fuzzing is still s-s.

Comment 1

[Christian Holler (:decoder)]

Attachment: Detailed Crash Information

Comment 2

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 3

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

There is a null check https://searchfox.org/mozilla-central/rev/b671b6390e88672543b9b7c82132be655bd98856/dom/ipc/ContentParent.cpp#7333

Comment 4

[Christian Holler (:decoder)]

Yes, but that one does not seem to be sufficient.

It looks like aContext.GetMaybeDiscarded()->Canonical() is null here.

Comment 5

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

Hmm, how is that possible? Is there some issue with MaybeDiscarded?
Oh, hmm, https://searchfox.org/mozilla-central/rev/b671b6390e88672543b9b7c82132be655bd98856/dom/ipc/MaybeDiscarded.h#108-110 hints that the API has rather unexpected behavior. I wonder in how many places we have similar issue.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Olli Pettay [:smaug] from comment #5)

Hmm, how is that possible? Is there some issue with MaybeDiscarded?
Oh, hmm, https://searchfox.org/mozilla-central/rev/b671b6390e88672543b9b7c82132be655bd98856/dom/ipc/MaybeDiscarded.h#108-110 hints that the API has rather unexpected behavior. I wonder in how many places we have similar issue.

Yeah, this is intentional. I originally didn't want to have MaybeDiscarded() as it's fundamentally racy whether or not you'll get a null response, but in some cases I needed it because there was some other mechanism guaranteeing that the BC is still alive. If you call MaybeDiscarded() you should always check the result.

Perhaps a more scary looking comment on that member saying "This member is NOT guaranteed to be non-null unless IsNullOrDiscarded() is false. It may be null even if IsNull() is false, as it could be a non-null BC reference which was already fully destroyed in this process"

Comment 7

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

I think we need to change the IsNull() API. !IsNull() so strongly hints that then thing isn't null.

Comment 8

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

So IsNull() is trying to capture the idea whether a non-null value was passed at all?
Could we rename it to something like WasPassed()?

Comment 9

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

Attachment: Bug 1761728, MaybeDiscarded::IsNull doesn't mean the value isn't null, r=peterv

Comment 10

[Nika Layzell [:nika] (ni? for response)]

(In reply to Olli Pettay [:smaug] from comment #8)

So IsNull() is trying to capture the idea whether a non-null value was passed at all?
Could we rename it to something like WasPassed()?

Yes, that is the intention, it checks whether a non-null value was passed at all.

I'm fine with renaming it to WasPassed() (and inverting the check, of course :p)

We'd also want to probably change IsNullOrDiscarded to NotPassedOrDiscarded().

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

MaybeDiscarded::IsNull doesn't mean the value isn't null, r=peterv
https://hg.mozilla.org/integration/autoland/rev/6867e23dd9628e8a725c714f1e1baa50e96f9896
https://hg.mozilla.org/mozilla-central/rev/6867e23dd962