OpenFileAndSendFDRunnable releases TabParent off the main thread

RESOLVED FIXED in mozilla37

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({crash})

unspecified
mozilla37
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [b2g-crash][caf-crash 216][caf priority: p2][CR 786377])

Attachments

(2 attachments, 2 obsolete attachments)

See bug 1101297.  This may not fix the crash but it should be fixed anyways.
Created attachment 8529254 [details] [diff] [review]
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 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.
Attachment #8529254 - Flags: review?(bent.mozilla)
Created attachment 8533412 [details] [diff] [review]
part 2 - Don't do anything in OpenFileAndSendFDRunnable::SendResponse() if opening failed.
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...
Attachment #8529254 - Flags: review?(bent.mozilla) → review+
Keywords: checkin-needed, leave-open
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=4500412&repo=mozilla-inbound
Flags: needinfo?(continuation)
Oops, I forgot to remove the assertion that Ben said would fail, and it failed. ;)
Flags: needinfo?(continuation)
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.
Attachment #8533412 - Flags: review?(bent.mozilla)
Keywords: leave-open
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 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.
Attachment #8533412 - Flags: review?(bent.mozilla)
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?
The first part just causes a different crash.
Created attachment 8542324 [details] [diff] [review]
OpenFileAndSendFDRunnable::OpenFile needs to ensure we don't release off main thread.

Actually address Ben's review comments.  Carrying forward his r+.
Attachment #8529254 - Attachment is obsolete: true
Attachment #8542324 - Flags: review+
Created attachment 8542338 [details] [diff] [review]
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.
Attachment #8533412 - Attachment is obsolete: true
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.
Flags: needinfo?(bent.mozilla)
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.
Attachment #8542338 - Flags: feedback+
(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?
Flags: needinfo?(bent.mozilla)
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
Attachment #8542338 - Flags: review?(bent.mozilla)
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
Attachment #8542338 - Flags: review?(bent.mozilla) → review+
> 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
https://hg.mozilla.org/mozilla-central/rev/7b8870e4821e
https://hg.mozilla.org/mozilla-central/rev/2dc60df56da1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37

Updated

4 years ago
Duplicate of this bug: 1101297
Whiteboard: [CR 786377]
Whiteboard: [CR 786377] → [caf priority: p2][CR 786377]
Whiteboard: [caf priority: p2][CR 786377] → [b2g-crash][caf-crash 216][caf priority: p2][CR 786377]
Keywords: crash
You need to log in before you can comment on or make changes to this bug.