1173219 - Make TabParent::RecvCreateWindow less crash prone

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

We do a bunch of things like this in TabParent::RecvCreateWindow:

nsresult rv = Foo();
NS_ENSURE_SUCCESS(rv, false);

This will crash the content process if rv is not a success code. That's pretty awful behaviour (I take the blame - this was early on in my e10s-ing).

We should probably send the nsresult back to the child instead.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1173219 - Never return false from TabParent::RecvCreateWindow to make opening windows more robust. r=billm

Bug 1173219 - Never return false from TabParent::RecvCreateWindow to make opening windows more robust. r?billm

We were returning false from TabParent::RecvCreateWindow whenever anything went wrong.
Returning false in response to an IPC message results in the content process being
killed, which is a terrible user experience. With this patch, instead of returning
false, we return the nsresult of operations occurring within TabParent.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/12205/#review10687

Hey billm - let me know if you'd rather I use Splinter for this.

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8627318 [details]
MozReview Request: Bug 1173219 - Never return false from TabParent::RecvCreateWindow to make opening windows more robust. r=billm

https://reviewboard.mozilla.org/r/12207/#review10805

Ship It\!

::: dom/ipc/TabParent.cpp:622
(Diff revision 1)
> -    return false;
> +    *aResult = NS_ERROR_UNEXPECTED;

Let's keep this as return false. There's pretty much no way that this can happen if the code is correct. It's more of a security check.

::: dom/ipc/TabChild.cpp:1541
(Diff revision 1)
> +    if (NS_WARN_IF(NS_FAILED(rv))) {

Given that we already warn when we set rv, I don't think we need to warn here as well.

::: dom/ipc/TabParent.cpp:619
(Diff revision 1)
> +  *aResult = NS_OK;

There are only two |return true| cases right now. Why don't we default to NS_ERROR_FAILURE or something and then set it to NS_OK right before the successful returns. That should eliminate a lot of |*aResult = NS_ERROR_FAILURE| assignments.

::: dom/ipc/TabParent.cpp:628
(Diff revision 1)
> -  NS_ENSURE_SUCCESS(rv, false);
> +  if (NS_WARN_IF(NS_FAILED(*aResult))) {

I think the style guide allows us to skip bracing these single-line error cases. Same comment applies to a bunch of branches below.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/12207/#review10805

> I think the style guide allows us to skip bracing these single-line error cases. Same comment applies to a bunch of branches below.

TIL we use K&R. Ok, good to know.

> There are only two |return true| cases right now. Why don't we default to NS_ERROR_FAILURE or something and then set it to NS_OK right before the successful returns. That should eliminate a lot of |*aResult = NS_ERROR_FAILURE| assignments.

My only reason for doing this is because I also use aResult to get results from various intermediate steps, which will return NS_OK, which essentially overwrites our default state.

We can, however, use this technique for the checks lower down in the file, so I've set \*aResult to NS_ERROR_FAILURE just before those.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #4)
> TIL we use K&R. Ok, good to know.

Not exactly. It's only for |return <error>;| lines where we would have done NS_ENSURE_TRUE previously. For non-errors, you need to brace even single-line conditionals.

That's my understanding at least.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8627318 [details]
MozReview Request: Bug 1173219 - Never return false from TabParent::RecvCreateWindow to make opening windows more robust. r=billm

Bug 1173219 - Never return false from TabParent::RecvCreateWindow to make opening windows more robust. r=billm

We were returning false from TabParent::RecvCreateWindow whenever anything went wrong.
Returning false in response to an IPC message results in the content process being
killed, which is a terrible user experience. With this patch, instead of returning
false, we return the nsresult of operations occurring within TabParent.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e17e313a3066

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e17e313a3066

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8627318 [details]
MozReview Request: Bug 1173219 - Never return false from TabParent::RecvCreateWindow to make opening windows more robust. r=billm

Approval Request Comment
[Feature/regressing bug #]:

None.

[User impact if declined]:

E10S users might suffer from more content process crashes.

[Describe test coverage new/current, TreeHerder]:

This has baked for a few days on central, and crash-stats is showing no crashes with the signature from bug 1180667, which this patch hopes to address.

[Risks and why]: 

Very low risk. This patch is very benign - it basically makes us return "true" for IPC message handlers more often, and return nsresults instead of "false" when things go wrong. Returning false, by design, causes child process aborts.

[String/UUID change made/needed]:

None.

Comment 10

[Kate Glazko]

Comment on attachment 8627318 [details]
MozReview Request: Bug 1173219 - Never return false from TabParent::RecvCreateWindow to make opening windows more robust. r=billm

Very low risk, this has baked for a few days on central with no issues.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/66660a969cdd

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1173219.html