1079546 - Rework slice handling for remote blobs

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

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

Right now we have some legacy behavior that we no longer need wrt sliced remote blobs. We can minimize messages and data copying in better ways now that Blob and BlobImpl have been split.

Comment 1

[David Flanagan [:djf]]

We're using some pretty hacky code in the gallery app (basically implementing a /tmp directory) to work around this bug. I'd really love to have it fixed in 2.2 so we can remove that hack. See bug 1063658.

Comment 2

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

This will be fixed long before 2.2 :)

Comment 3

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

Attachment: Patch, v1

This patch makes the following changes:

1. Parent blobs always own a sliceable blob (no more holding onto serialized input streams).
2. Parent blob slices do not ever create new actors.
3. Child blob slices do not create new actors unless and until those slices are sent to the parent.
4. Child blob slices can ask for a sliced portion of the parent's data when GetInternalStream is called.

Comment 4

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

Attachment: Code cleanup (no functional changes), v1

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Woo!

Based on our activity idiom concerns, is it possible to add a test whose steps are like this (effectively smooshing the two tests together):
- Create mozbrowser child process A, it creates a Blob and sends it to the parent.
- Parent holds onto the Blob.
- Remove/kill mozbrowser A, ideally making sure it's dead dead.
- Create mozbrowser B (this could happen earlier)
- Send the from-A Blob down to the mozbrowser B and verify it like in the existing from_parent_process test.

This may end up logically equivalent, but it'd be nice to have the "the original process" died thing which has frequently been an important STR for the "pick" web activity problems we've been having.

Comment 6

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

Comment on attachment 8503404 [details] [diff] [review]
Patch, v1

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

::: dom/ipc/Blob.cpp
@@ +875,5 @@
> +  nsRefPtr<FileImpl> blobImpl;
> +
> +  if (auto length = static_cast<size_t>(aParams.data().Length())) {
> +    if (!aMetadata.mHasRecursed &&
> +        aMetadata.IsFile() &&

Remove this check.

@@ +889,5 @@
> +
> +    memcpy(buffer, aParams.data().get(), length);
> +
> +    if (!aMetadata.mHasRecursed && aMetadata.IsFile()) {
> +      if (NS_WARN_IF(aMetadata.mLength != uint64_t(length))) {

This is not needed since above check does the same thing

@@ +1098,5 @@
> +
> +  nsTArray<nsRefPtr<FileImpl>> blobImpls;
> +  fallibleBlobImpls.SwapElements(blobImpls);
> +
> +  bool hasRecursed = aMetadata.mHasRecursed;

const

@@ +2032,5 @@
> +                                         uint64_t aStart,
> +                                         uint64_t aLength,
> +                                         const nsAString& aContentType)
> +  : RemoteBlobImpl(aContentType, aLength)
> +  , mParent(aParent)

Make this point to root RemoteBlobImpl

Comment 7

[Andrew Overholt [:overholt]]

With comment 2 in mind, let's just mark this 2.2+ so it's clear it was/is a priority and will be fixed if things regress.  Bookkeeping++ :)

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/10a1daff3c47

Comment 9

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

(In reply to Andrew Sutherland [:asuth] from comment #5)
> Based on our activity idiom concerns, is it possible to add a test

I didn't get around to this but I will try to add one when I get some time.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/10a1daff3c47

Comment 11

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

Comment on attachment 8503404 [details] [diff] [review]
Patch, v1

Approval Request Comment
[Feature/regressing bug #]: 994190
[User impact if declined]: Common use of DOM blobs will crash
[Describe test coverage new/current, TBPL]: Good platform tests added here
[Risks and why]: Low risk, and it only missed uplift by a few days.
[String/UUID change made/needed]: None

Comment 12

[David Flanagan [:djf]]

We uplifted a Gaia-based workaround for this bug to Gaia v2.0 and 2.1 which use Gecko 32 and 34. The tracking flags for this bug have Gecko 34 and earlier listed an unaffected.  Is that actually true, or are those releases affected but not fixable with this patch? Could this patch be upfifted to the b2g branch of gecko 34? Do we actually need a gaia workaround in 2.0 and 2.1?

Comment 13

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

Bug 994190 only landed on 35, so this patch can't land as-is on earlier branches. I suppose that this could be reworked for earlier branches if needed, but if we have workarounds in place I don't know if it is worth the effort.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 17

[David Flanagan [:djf]]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #13)
> Bug 994190 only landed on 35, so this patch can't land as-is on earlier
> branches. I suppose that this could be reworked for earlier branches if
> needed, but if we have workarounds in place I don't know if it is worth the
> effort.

Andrew and Julien: since your apps were affected by this bug, what's your feeling about the desireability of getting it fixed in gecko in 2.1.  It is awfully late now, and we do have a workaround for Gallery. But any third party app that allows apps to pick memory-backed image blobs will cause the same crash.  I don't know how likely that is in practice, of course.

Comment 18

[Julien Wajsberg [:julienw]]

It's unclear whether the patch will fix the issue for v2.1.

Comment 19

[Julien Wajsberg [:julienw]]

Sorry, I pressed "save" before finishing the message ;)

So: it's unclear whether the patch will fix the issue for v2.1, given the fact that bug 994190 is not in v2.1. I think a patch suitable for v2.1 would need to be completely different, and would have considerable risk.

On the other hand, v2.1 will likely stay for some months, and having such a bug in v2.1 is quite bad.

I'll go along with the opinion of Ben here, because anyway he'll be the one to do the patch for v2.1 :)

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dd9b0aedcf05

Comment 23

[jbolton]

Cannot verify this here- seems like a back-end fix.