Fetch should reuse XHR's 'store large blobs in temp files' -setup

RESOLVED FIXED in Firefox 52

Status

()

Core
DOM
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: smaug, Assigned: baku)

Tracking

50 Branch
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
See bug 1202006
(Assignee)

Updated

a year ago
Assignee: nobody → amarchesini
It really feels like we should do this for all blobs regardless of how they are produced.
(Reporter)

Comment 2

a year ago
maybe.  If we are keeping the data anyhow alive somewhere else, no need to create a copy of it as a tmp file.
(In reply to Olli Pettay [:smaug] from comment #2)
> maybe.  If we are keeping the data anyhow alive somewhere else, no need to
> create a copy of it as a tmp file.

Are you going to persist to disk when that last other thing gets GC'd?  That would be fine.  I think, though, just putting large things in a temp file immediately would be simpler and have little downside.

But we should be consistent that "large blobs are file backed" so devs can use them for seeking access to huge things without worrying about OOMs.  Right now they have to memorize which magic calls happen to do that file-backed behavior and from the folks I've talked to know one really understands it.
(Assignee)

Comment 4

a year ago
Created attachment 8804148 [details] [diff] [review]
part 1 - MutableBlobStreamListener
Attachment #8804148 - Flags: review?(bugs)
(Assignee)

Comment 5

a year ago
Created attachment 8804149 [details] [diff] [review]
part 2 - tests
Attachment #8804149 - Flags: review?(bugs)
(Reporter)

Comment 6

a year ago
Comment on attachment 8804148 [details] [diff] [review]
part 1 - MutableBlobStreamListener

I heard a rumor that qDot could do some reviews :)
Want to take a look a this? If not, put it back to my queue.
Attachment #8804148 - Flags: review?(bugs) → review?(kyle)
(Reporter)

Updated

a year ago
Attachment #8804149 - Flags: review?(bugs) → review?(kyle)
Comment on attachment 8804148 [details] [diff] [review]
part 1 - MutableBlobStreamListener

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

LGTM (after much other code digging to see how this worked with XHR heh), and glad I caught this review as it could be interesting to route this to encrypted disk storage once we get that done for IDB blobs, instead of just going directly to memory in PB mode.
Attachment #8804148 - Flags: review?(kyle) → review+

Comment 8

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8044e281e67c
Fetch API should use MutableBlobStorage to store big Blobs on disk, r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/18e1ee2de339
Tests for Fetch API + MutableBlobStorage, r=qdot

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8044e281e67c
https://hg.mozilla.org/mozilla-central/rev/18e1ee2de339
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.