1105468 - OpenFileAndSendFDRunnable releases TabParent off the main thread

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Andrew McCreight [:mccr8]]

See bug 1101297.  This may not fix the crash but it should be fixed anyways.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: OpenFileAndSendFDRunnable::OpenFile needs to ensure we don't release off main thread.

This should fix the various ways we can end up destroying the runnable off the main thread when OpenFile fails.

One perhaps questionable decision here is to leak rather than crash if the dispatch fails.

I'm not sure what runs this code, so I haven't tested it.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=399e7fec61ad

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8529254 [details] [diff] [review]
OpenFileAndSendFDRunnable::OpenFile needs to ensure we don't release off main thread.

There are a bunch of bc failures, but those are just from the revision I used of m-c.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Don't do anything in OpenFileAndSendFDRunnable::SendResponse() if opening failed.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8529254 [details] [diff] [review]
OpenFileAndSendFDRunnable::OpenFile needs to ensure we don't release off main thread.

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

::: dom/ipc/TabParent.cpp
@@ +116,5 @@
>  
>  private:
>      ~OpenFileAndSendFDRunnable()
>      {
> +        MOZ_ASSERT(NS_IsMainThread());

This can't be asserted because the final release may happen on a background thread.

@@ +198,5 @@
>  
> +            // Intentionally leak the runnable (but not the fd) rather
> +            // than crash when trying to release a main thread object
> +            // off the main thread.
> +            NS_ADDREF(this);

I'd just do mTabParent.forget() here...

Comment 5

[Carsten Book [:Tomcat]]

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

Comment 6

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4500412&repo=mozilla-inbound

Comment 7

[Andrew McCreight [:mccr8]]

Oops, I forgot to remove the assertion that Ben said would fail, and it failed. ;)

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8533412 [details] [diff] [review]
part 2 - Don't do anything in OpenFileAndSendFDRunnable::SendResponse() if opening failed.

Apparently this fixes the new crash.

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8533412 [details] [diff] [review]
part 2 - Don't do anything in OpenFileAndSendFDRunnable::SendResponse() if opening failed.

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

Oh this is really weird... I would expect this to make the child wait forever? Any idea why it doesn't?

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8533412 [details] [diff] [review]
part 2 - Don't do anything in OpenFileAndSendFDRunnable::SendResponse() if opening failed.

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #9)
> Oh this is really weird... I would expect this to make the child wait
> forever? Any idea why it doesn't?
Hmm, ok, that probably just indicates I need to test it more and read the code some more.

Comment 11

[Johnny Stenback (:jst)]

Andrew, can we get an updated patch for the first part of this landed? Or is there a reason not to land that fix alone?

Comment 12

[Andrew McCreight [:mccr8]]

The first part just causes a different crash.

Comment 13

[Andrew McCreight [:mccr8]]

Attachment: OpenFileAndSendFDRunnable::OpenFile needs to ensure we don't release off main thread.

Actually address Ben's review comments.  Carrying forward his r+.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: In SendResponse, send an invalid FD to the child if we failed to open the file. WIP

I read over this code a little more and I think I understand what bent was saying. This new version sends off an invalid FileDescriptor to the child if we didn't get an fd.

I don't know if that's the right approach, but it is less bad.  The only alternative I can think of is to create a new message to send to the child to indicate failure.

I verified that we can at least fire off this runnable with an invalid file and things don't go haywire, but I'm still not sure how to test out the actual app loading path.

Comment 15

[Andrew McCreight [:mccr8]]

Ben, is there some way I can test this?  Even if it is just something like modifying a test case and doing a B2G emulator try build or something.

Comment 16

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8542338 [details] [diff] [review]
In SendResponse, send an invalid FD to the child if we failed to open the file. WIP

This looks ok to me... It will cause the child to die, but that's better than other alternatives I think.

Comment 17

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Andrew McCreight [:mccr8] from comment #15)
> Ben, is there some way I can test this?

I can't think of a reason that this should happen, so I can't think of a way to test it really... You could break in a debugger and manually take the new path I guess?

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 8542338 [details] [diff] [review]
In SendResponse, send an invalid FD to the child if we failed to open the file. WIP

I didn't end up testing what happens in the child.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7d4cbdd269ce

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8542338 [details] [diff] [review]
In SendResponse, send an invalid FD to the child if we failed to open the file. WIP

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

This looks fine, hopefully this doesn't just move the problem elsewhere :)

::: dom/ipc/TabParent.cpp
@@ +161,5 @@
> +            mozilla::unused << tabParent->SendCacheFileDescriptor(mPath, fd);
> +        }
> +
> +        if (!mFD) {
> +          return;

Nit: 4 space indent

Comment 20

[Andrew McCreight [:mccr8]]

> Nit: 4 space indent
Fixed.  I should file a bug about updating the mode line or updating the indentation to be standard.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7b8870e4821e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2dc60df56da1

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b8870e4821e
https://hg.mozilla.org/mozilla-central/rev/2dc60df56da1