Rework slice handling for remote blobs

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bent.mozilla, Assigned: bent.mozilla)

Tracking

Trunk
mozilla36
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox33 unaffected, firefox34 unaffected, firefox35 fixed, firefox36 fixed, b2g-v2.2 fixed)

Details

Attachments

(2 attachments)

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.
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.
blocking-b2g: --- → 2.2?
This will be fixed long before 2.2 :)
Blocks: 1080104
Posted patch Patch, v1Splinter Review
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.
Attachment #8503404 - Flags: review?(khuey)
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 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
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++ :)
blocking-b2g: 2.2? → 2.2+
(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.
https://hg.mozilla.org/mozilla-central/rev/10a1daff3c47
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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
Attachment #8503404 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(bent.mozilla)
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.
Flags: needinfo?(bent.mozilla)
Duplicate of this bug: 1080020
Attachment #8503404 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1079824
Needs rebasing for Aurora uplift.
Flags: needinfo?(bent.mozilla)
(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.
Flags: needinfo?(felash)
Flags: needinfo?(bugmail)
It's unclear whether the patch will fix the issue for v2.1.
Flags: needinfo?(felash)
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 :)
Duplicate of this bug: 1082226
Duplicate of this bug: 921812

Comment 23

5 years ago
Cannot verify this here- seems like a back-end fix.
QA Whiteboard: [QAnalyst-Triage?], [QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?], [QAnalyst-verify-] → [QAnalyst-Triage+], [QAnalyst-verify-]
Flags: needinfo?(ktucker)
Flags: needinfo?(bugmail)
You need to log in before you can comment on or make changes to this bug.