Closed Bug 1423913 Opened 2 years ago Closed 2 years ago

Another (potential) Crash in nsDocShell::MaybeCreateInitialClientSource

Categories

(Core :: DOM: Navigation, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: mkaply, Assigned: bkelly)

References

Details

Attachments

(5 files, 1 obsolete file)

Attached image Callstack
In bug 1372107 we're investigating allowing the browser to start after showing an error about AutoConfig.

After bug 1419536 landed, this crashes. Call stack is similar to 1420743 but not the same (attached).

To recreate, remove the browser quit in nsReadConfig

https://searchfox.org/mozilla-central/source/extensions/pref/autoconfig/src/nsReadConfig.cpp#105

And add the following files after building Firefox:

defaults/pref/autoconfig.js:

pref('general.config.filename', 'firefox.cfg');
pref('general.config.obscure_value', 0);

firefox.cfg

//
JAVASCRIPT ERROR
See Also: → 1420743
Mike, are you hitting an assertion?  Do you know what URL you are trying to load in the docshell here?
Flags: needinfo?(mozilla)
Yes, I'm hitting the MOZ_DIAGNOSTIC_ASSERT at line 3404 (mScriptGlobal->GetCurrentInnerWindowInternal()->GetClientInfo().isSome());

I see nsNestedAboutURI in the call stack. My guess is it is trying to load about:sessionrestore.
Flags: needinfo?(mozilla)
about:blank.
I'll investigate.  I'm thinking maybe I should switch ClientSource creation to be infallible, even during shutdown.  Any ClientSource created during shutdown would just be detached without a backing actor instead.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attached file Patch for readConfig
Here's the code patch to just start Firefox when the failure happens after the popup.

To get the popup requires that you add two files to a Firefox install.

The first file goes in defaults/pref where the EXE is installed. On Mac, it's Content/Resources/defaults/pref inside the .app file. That file should contain:

pref('general.config.filename', 'firefox.cfg');
pref('general.config.obscure_value', 0);

The second file you need is firefox.cfg.

That file goes where the EXE is located or at Contents/Resources in the .app on Mac.

It contains:

// First line
Text to cause error

With those two files, you should see the error at startup. Firefox.cfg is just a Javascript file and we are causing the error at startup.
I tested the steps in comment 5.  I got the dialog on startup and the browser started fine after that with my P1/P2 patches applied.
Comment on attachment 8935423 [details] [diff] [review]
P1 Make ClientManager::CreateSource() and CreateHandle() infallible.  Errors result in a detached object instead of nullptr.  r=baku

Andrea, it seems there are some paths that expect to create windows in cases where we think we are shutdown.  Right now my code is both asserting and failing window creation in these cases.  We should probably allow the browser to open these windows anyway even if the clients API is shutdown.

To do this I propose that we make ClientManager::CreateSource() and ClientManager::CreateHandle() infallible.  If they encounter an error they will simply shutdown the ClientSource or ClientHandle.  This will result in a detached object without a backing IPC actor.  Things like GetClientInfo() will work, but remote operations will fail.
Attachment #8935423 - Flags: review?(amarchesini)
Comment on attachment 8935424 [details] [diff] [review]
P2 Make callers expect infallble CreateSource() and CreateHandle(). r=baku

This also makes it nicer to use CreateSource() and CreateHandle().  We can just assert that they return an object and have less branching in the code.
Attachment #8935424 - Flags: review?(amarchesini)
I also investigated a bit more to see what was failing in this case.

It turns out the problem was that in this path we have a browser running for a short time with zero windows.  The initial dialog created a ClientManagerService, then its gone and the service is destroyed.  When the next window creates another service instance the shutdown handler was failing to register.  This caused us to shutdown the service immediately.

This patch should fix that problem by only registering one shutdown handler even if we end up with multiple services instantiated over time.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2368b9ffa6ed50ad60a1f2d9b7c06f6ce8f33d4
Attachment #8935443 - Flags: review?(amarchesini)
Blocks: 1372107
Blocks: 1422823
See Also: 1422823
Comment on attachment 8935423 [details] [diff] [review]
P1 Make ClientManager::CreateSource() and CreateHandle() infallible.  Errors result in a detached object instead of nullptr.  r=baku

Review of attachment 8935423 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/clients/manager/ClientManager.cpp
@@ +108,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
> +    // If we can't even get a UUID, at least make sure not to use a garbage
> +    // value.  Instead return a shutdown ClientSource with a zero'd id.
> +    // This should be exceptionally rare, if it happens at all.
> +    id.Clear();

make an union instead and return a void_t ?
Attachment #8935423 - Flags: review?(amarchesini) → review+
Attachment #8935424 - Flags: review?(amarchesini) → review+
Attachment #8935443 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini [:baku] from comment #13)
> make an union instead and return a void_t ?

I'm not sure how I could easily do this without a bunch of other refactoring.  For now I will add a MOZ_DIAGNOSTIC_ASSERT() to verify that this never really happens for now.  (I had originally made it a MOZ_CRASH, but figured this might be a bit safer.)
Pushed by bkelly@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79b9a988f0fd
P1 Make ClientManager::CreateSource() and CreateHandle() infallible.  Errors result in a detached object instead of nullptr.  r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b07d14b67b
P2 Make callers expect infallble CreateSource() and CreateHandle(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/c71de7f98d08
P3 Don't register more shutdown handle if we create more than one ClientManagerService instance. r=baku
https://hg.mozilla.org/mozilla-central/rev/79b9a988f0fd
https://hg.mozilla.org/mozilla-central/rev/f6b07d14b67b
https://hg.mozilla.org/mozilla-central/rev/c71de7f98d08
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.