Closed Bug 1173219 Opened 5 years ago Closed 4 years ago

Make TabParent::RecvCreateWindow less crash prone

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
e10s m8+ ---
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Assignee: nobody → mconley
Status: NEW → ASSIGNED
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.
Attachment #8627318 - Flags: review?(wmccloskey)
https://reviewboard.mozilla.org/r/12205/#review10687

Hey billm - let me know if you'd rather I use Splinter for this.
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.
Attachment #8627318 - Flags: review?(wmccloskey) → review+
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.
(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 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.
Attachment #8627318 - Attachment description: MozReview Request: Bug 1173219 - Never return false from TabParent::RecvCreateWindow to make opening windows more robust. r?billm → MozReview Request: Bug 1173219 - Never return false from TabParent::RecvCreateWindow to make opening windows more robust. r=billm
Attachment #8627318 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e17e313a3066
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
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.
Attachment #8627318 - Flags: approval-mozilla-aurora?
Blocks: 1180667
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.
Attachment #8627318 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.