Closed Bug 1420743 Opened 3 years ago Closed 3 years ago

Crash in nsDocShell::MaybeCreateInitialClientSource


(Core :: DOM: Navigation, defect, P2)

Windows 10



Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- unaffected
firefox59 --- fixed


(Reporter: jseward, Assigned: bkelly)



(Keywords: crash)

Crash Data


(2 files, 2 obsolete files)

This bug was filed from the Socorro interface and is
report bp-ae6a5348-b202-4f16-909c-219540171125.

This is topcrash #17 in the windows nightly 20171123220110.

Top 10 frames of crashing thread:

0 xul.dll nsDocShell::MaybeCreateInitialClientSource docshell/base/nsDocShell.cpp:3406
1 xul.dll nsDocShell::DoChannelLoad docshell/base/nsDocShell.cpp:11844
2 xul.dll nsDocShell::DoURILoad docshell/base/nsDocShell.cpp:11645
3 xul.dll nsDocShell::InternalLoad docshell/base/nsDocShell.cpp:10977
4 xul.dll nsDocShell::OnLinkClickSync docshell/base/nsDocShell.cpp:14605
5 xul.dll OnLinkClickEvent::Run docshell/base/nsDocShell.cpp:14347
6 xul.dll mozilla::SchedulerGroup::Runnable::Run xpcom/threads/SchedulerGroup.cpp:396
7 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1037
8 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:97
9 xul.dll mozilla::ipc::MessagePumpForChildProcess::Run ipc/glue/MessagePump.cpp:301

Flags: needinfo?(bzbarsky)
Blocks: 1419536
Flags: needinfo?(bzbarsky) → needinfo?(bkelly)
I'll look at this first thing in the morning.  Must be hitting some path we don't test in automation.
If there are any URLs associated with these crashes, it would be nice to know in order to try to reproduce.
I may just relax this assertion to a MOZ_ASSERT() and clear mInitialClientSource for now.
I guess its possible a link click occurs while already loading and we end up calling LinkClickSync() before we do this for the first load:

I think perhaps I should always take the initial client source in here, even if we don't keep it:

Also, moving the reset of mInitialClientSource up in nsDocShell::EndPageLoad() would be good.  We can leave it in place if we hit any of those error paths.

I guess I should try those things first and see if it helps the crash rate.
Crash Signature: [@ nsDocShell::MaybeCreateInitialClientSource] → [@ nsDocShell::MaybeCreateInitialClientSource] [@ nsGlobalWindowInner::GetClientInfo const] [@ nsGlobalWindowInner::GetClientInfo]
There are actually two different kind of crashes in the reports attached here.  I'll add a second patch to try to address the less frequent crashes as well.
Andrea, this patch tries to fix the crash in comment 0 by doing a better job of clear nsDocShell::mInitialClientSource.  I believe the crash is due to leaving the field populated in some cases which then triggers the assertion on the next navigation.

The patch does this in two ways:

1. Make the inner window always call nsDocShell::TakeClientSource() to consume the value even if it ends up using the reserved client from the load info.

2. Clear the value in nsDocShell::EndPageLoad() first thing to avoid leaving the field populated if we take an early exit for an error.
Attachment #8932087 - Attachment is obsolete: true
Attachment #8932095 - Flags: review?(amarchesini)
This patch attempts to further address the comment 0 crash and the other crashes by correcting the conditional in nsDocShell::EnsureInitialClientSource().  We store the ClientSource on the inner window, yet the code is currently checking the outer window's existence and extant doc instead.  If there are any cases where the outer window and the inner window are briefly out of sync and we get called here (due to GetDocument()) then we will trigger the assertions.

Lets avoid the possibility of out-of-sync outer/inner windows by only checking the inner window instead.  Since that is where we start the ClientSource its the state we are really interested in here.
Attachment #8932097 - Flags: review?(amarchesini)
Priority: -- → P2
Comment on attachment 8932095 [details] [diff] [review]
P1 Do a better job of clearing docshell's mInitialClientSource at the end of page load. r=baku

Review of attachment 8932095 [details] [diff] [review]:

::: docshell/base/nsDocShell.cpp
@@ +7799,5 @@
>  {
> +  // Make sure to discard the initial client if we never created the initial
> +  // about:blank document.  Do this before possibly returning from the method
> +  // due to an error.
> +  mInitialClientSource.reset();

Move it after line 7807.
Attachment #8932095 - Flags: review?(amarchesini) → review+
Attachment #8932097 - Flags: review?(amarchesini) → review+
Move the mInitialClientSource.reset() down as requested.

I also, however, added code to clear the mInitialClientSource if the channel load or open fails.  I discussed this with Andrea in IRC.
Attachment #8932095 - Attachment is obsolete: true
Attachment #8932548 - Flags: review+
Pushed by
P1 Do a better job of clearing docshell's mInitialClientSource at the end of page load. r=baku
P2 Check the inner window for an existing document instead of the outer window in nsDocShell::EnsureInitialClientSource(). r=baku
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Version: unspecified → Trunk
Someone in Mac crashed using 20171206100053 build with signature nsDocShell::MaybeCreateInitialClientSource. Should I file a new bug for that crash? Crash reason: MOZ_RELEASE_ASSERT(mScriptGlobal->GetCurrentInnerWindowInternal()->GetClientInfo().isSome())
(In reply to Marcia Knous [:marcia - use ni] from comment #14)
> Someone in Mac crashed using 20171206100053 build with signature
> nsDocShell::MaybeCreateInitialClientSource. Should I file a new bug for that
> crash? Crash reason:
> MOZ_RELEASE_ASSERT(mScriptGlobal->GetCurrentInnerWindowInternal()-
> >GetClientInfo().isSome())

Yes, please file a new bug.  Thanks.
See Also: → 1423913
You need to log in before you can comment on or make changes to this bug.