Closed Bug 1619953 Opened 5 years ago Closed 4 years ago

Extend BlobURLChannel to always request and read BlobURL data from parent process

Categories

(Core :: DOM: File, enhancement, P3)

Product:
Core
Component:
DOM: File
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
81 Branch
Tracking Flags:
Tracking Status
firefox81 --- fixed

People

(Reporter: ssengupta, Assigned: ssengupta, Mentored)

References

(Blocks 2 open bugs)

Details

Attachments

(12 files, 3 obsolete files)

Bug 1619953 - P1 - Added missing include in PChromiumCDM.ipdl r=baku
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619953 - P2 - Asynchronous BlobURLDataRequest introduced r=#dom-workers-and-storage,baku
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619953 - P3 - Asynchronous BlobURLInputStream added r=#dom-workers-and-storage,baku
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619953 - P4 - BlobURLChannel now creates BlobURLInputStream to acquire blob data r=#dom-workers-and-storage,baku
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619953 - P5 - BlobURLChannel allows loading blob data that is very recently revoked r=#dom-workers-and-storage,baku
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619953 - P6 - BlobURLChannel creates BlobURLInputStream only when content stream is opened r=#dom-workers-and-storage,baku
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619953 - P7 - mozJSSubScriptLoader::ReadScript() now attempts reading from input stream even if content length is not already known r=#dom-workers-and-storage,baku
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619953 - P8 - Test `browser_blob-channelname.js` converted to xpcshell-test and updated to work with asynchronous input streams r=#dom-workers-and-storage,baku
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619953 - P9 - Test `browser_ContentSearch.js` disabled for linux64debug when fission not enabled r=baku
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619953 - P10 - Test `browser_privatebrowsing_blobUrl.js` modified to pass on win32 r=baku
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619953 - P11 - Mochitest `test_presentation_dc_sender.html` disabled for e10s r=mccr8
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619953 - P12 - Blob data is retrieved locally for non-http/https URLs r=baku
47 bytes, text/x-phabricator-request
Details | Review

This is an unobtrusive first step to eliminate broadcast of BlobURLs between content processes. Currently the BlobURL information is shared between hash tables owned by parent process and various content processes. Always requesting the parent process for BlobURLs will render the hash tables owned by content processes needless.

Very brief outline of a possible implementation:

  • BlobURLProtocolHandler should no longer initialize BlobURLChannel with BlobImpl pointer, because unless it is already the parent process, BlobImpl should not be locally available at this stage.

  • The initialization of BlobURLChannel should create a new asynchronous input stream, e.g., BlobURLInputStream.

  • Upon creation, this input stream should request the parent process for a BlobURL over IPC. If parent process responds with success, the input stream will have information on IPCBlob and the corresponding principal.

  • This information can be later used to retrieve actual BlobImpl data by whichever entity wishes to read from the input stream using asyncWait, passing a callback function. In case an input stream is created and not read from, no IPC calls will be generated.

Blocks: 1619943
Priority: -- → P3
Attachment #9133809 - Attachment is obsolete: true
Attachment #9133810 - Attachment is obsolete: true
Depends on: 1637742
Severity: normal → S3

This fix was required for subsequent patches to build successfully, because adding new source files exposed missing dependencies in unified sources.

The content process should use this method to send blob url and triggering principal to the parent process and expect blobImpl in return, if the blob is found and the triggering principal subsumes the blob's principal.

Depends on D75290

Reading from this stream type should be the preferred way to consume data from blob url. When created in the parent process, it requests blob data locally from BlobURLProtocolHandler. When created in a content process, it makes BlobURLDataRequest IPC call to the parent process. Should be wrapped in a nsBufferedInputStream, because ReadSegments() is currently not implemented for this stream type.

Depends on D75291

BlobURLProtocolHandler no longer passes blobImpl pointer to BlobURLChannel. Instead, acquisition and handling of blob data is offloaded to BlobURLInputStream.

Depends on D75292

Depends on: 1641825, 1641826

Previously similar logic existed in BlobURLProtocolHandler, which has now been removed, since such checks are now for parent process only and should be abstracted from BlobURLProtocolHandler.

Depends on D75293

Additionally, BlobURLChannel holds no strong reference to BlobURLInputStream. This is to avoid circular dependency.

Depends on D81126

If reading succeeds, it is checked that the length is known from that point on (this check is debug only).

Depends on D81452

Attachment #9148328 - Attachment description: Bug 1619953 - P2 - Asynchronous BlobURLDataRequest introduced r=baku → Bug 1619953 - P2 - Asynchronous BlobURLDataRequest introduced r=baku,#dom-workers-and-storage
Attachment #9148329 - Attachment description: Bug 1619953 - P3 - Asynchronous BlobURLInputStream added r=baku → Bug 1619953 - P3 - Asynchronous BlobURLInputStream added r=baku,#dom-workers-and-storage
Attachment #9148330 - Attachment description: Bug 1619953 - P4 - BlobURLChannel now creates BlobURLInputStream to acquire blob data r=baku → Bug 1619953 - P4 - BlobURLChannel now creates BlobURLInputStream to acquire blob data r=baku,#dom-workers-and-storage
Attachment #9159349 - Attachment description: Bug 1619953 - P5 - BlobURLChannel allows loading blob data that is very recently revoked r=baku → Bug 1619953 - P5 - BlobURLChannel allows loading blob data that is very recently revoked r=baku,#dom-workers-and-storage
Attachment #9159919 - Attachment description: Bug 1619953 - P6 - BlobURLChannel creates BlobURLInputStream only when content stream is opened r=baku → Bug 1619953 - P6 - BlobURLChannel creates BlobURLInputStream only when content stream is opened r=baku,#dom-workers-and-storage
Attachment #9160406 - Attachment description: Bug 1619953 - P7 - mozJSSubScriptLoader::ReadScript() now attempts reading from input stream even if content length is not already known r=baku → Bug 1619953 - P7 - mozJSSubScriptLoader::ReadScript() now attempts reading from input stream even if content length is not already known r=baku,#dom-workers-and-storage
Attachment #9160515 - Attachment description: Bug 1619953 - P8 - Mochitest `browser_blob-channelname.js` updated to work with asynchronous input streams r=baku → Bug 1619953 - P8 - Test `browser_blob-channelname.js` converted to xpcshell-test and updated to work with asynchronous input streams r=baku,#dom-workers-and-storage

This is done because the test always suffers from shutdown leaks after the changes in Bug 1619953, which make the stream acquired during the test asynchronous.

Depends on D81783

Attachment #9148328 - Attachment description: Bug 1619953 - P2 - Asynchronous BlobURLDataRequest introduced r=baku,#dom-workers-and-storage → Bug 1619953 - P2 - Asynchronous BlobURLDataRequest introduced r=#dom-workers-and-storage,baku
Attachment #9148329 - Attachment description: Bug 1619953 - P3 - Asynchronous BlobURLInputStream added r=baku,#dom-workers-and-storage → Bug 1619953 - P3 - Asynchronous BlobURLInputStream added r=#dom-workers-and-storage,baku
Attachment #9148330 - Attachment description: Bug 1619953 - P4 - BlobURLChannel now creates BlobURLInputStream to acquire blob data r=baku,#dom-workers-and-storage → Bug 1619953 - P4 - BlobURLChannel now creates BlobURLInputStream to acquire blob data r=#dom-workers-and-storage,baku
Attachment #9159349 - Attachment description: Bug 1619953 - P5 - BlobURLChannel allows loading blob data that is very recently revoked r=baku,#dom-workers-and-storage → Bug 1619953 - P5 - BlobURLChannel allows loading blob data that is very recently revoked r=#dom-workers-and-storage,baku
Attachment #9159919 - Attachment description: Bug 1619953 - P6 - BlobURLChannel creates BlobURLInputStream only when content stream is opened r=baku,#dom-workers-and-storage → Bug 1619953 - P6 - BlobURLChannel creates BlobURLInputStream only when content stream is opened r=#dom-workers-and-storage,baku
Attachment #9160406 - Attachment description: Bug 1619953 - P7 - mozJSSubScriptLoader::ReadScript() now attempts reading from input stream even if content length is not already known r=baku,#dom-workers-and-storage → Bug 1619953 - P7 - mozJSSubScriptLoader::ReadScript() now attempts reading from input stream even if content length is not already known r=#dom-workers-and-storage,baku
Attachment #9160515 - Attachment description: Bug 1619953 - P8 - Test `browser_blob-channelname.js` converted to xpcshell-test and updated to work with asynchronous input streams r=baku,#dom-workers-and-storage → Bug 1619953 - P8 - Test `browser_blob-channelname.js` converted to xpcshell-test and updated to work with asynchronous input streams r=#dom-workers-and-storage,baku
Blocks: 1647710
Pushed by ncsoregi@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b79dc688bfc9 P1 - Added missing include in PChromiumCDM.ipdl r=baku https://hg.mozilla.org/integration/autoland/rev/77c24528c8c2 P2 - Asynchronous BlobURLDataRequest introduced r=baku https://hg.mozilla.org/integration/autoland/rev/4668c99560de P3 - Asynchronous BlobURLInputStream added r=baku https://hg.mozilla.org/integration/autoland/rev/fccda4625d51 P4 - BlobURLChannel now creates BlobURLInputStream to acquire blob data r=baku https://hg.mozilla.org/integration/autoland/rev/830eb658d70e P5 - BlobURLChannel allows loading blob data that is very recently revoked r=baku https://hg.mozilla.org/integration/autoland/rev/062c0c9b132f P6 - BlobURLChannel creates BlobURLInputStream only when content stream is opened r=baku https://hg.mozilla.org/integration/autoland/rev/38aac5691967 P7 - mozJSSubScriptLoader::ReadScript() now attempts reading from input stream even if content length is not already known r=baku https://hg.mozilla.org/integration/autoland/rev/4e66983a4f00 P8 - Test `browser_blob-channelname.js` converted to xpcshell-test and updated to work with asynchronous input streams r=baku,necko-reviewers https://hg.mozilla.org/integration/autoland/rev/9f610c8c44de P9 - Test `browser_ContentSearch.js` disabled for linux64debug when fission not enabled r=baku

Backed out for causing leaks.

backout: https://hg.mozilla.org/integration/autoland/rev/16c92f73e178c26d35c998aac18e24f366400c5b

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=9f610c8c44de2797e1b32ca92eb6a6c83c884a6d&searchStr=browser-chrome&selectedTaskRun=bnBcQNNsRESQ4_f-21Gn4A.0

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308327207&repo=autoland&lineNumber=30172

[...]
[task 2020-07-02T14:38:36.686Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 3 imgRequestProxy
[task 2020-07-02T14:38:36.686Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 3 nsBaseChannel
[task 2020-07-02T14:38:36.686Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 1 nsDocShell::InterfaceRequestorProxy
[task 2020-07-02T14:38:36.686Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 3 nsInputStreamPump
[task 2020-07-02T14:38:36.686Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 2 nsJSPrincipals
[task 2020-07-02T14:38:36.686Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 3 nsLoadGroup
[task 2020-07-02T14:38:36.686Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 1 nsNodeWeakReference
[task 2020-07-02T14:38:36.686Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 3 nsProgressNotificationProxy
[task 2020-07-02T14:38:36.686Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 3 nsProperties
[task 2020-07-02T14:38:36.686Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 5 nsSimpleURI
[task 2020-07-02T14:38:36.686Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 29 nsStringBuffer
[task 2020-07-02T14:38:36.687Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 39 nsTArray_base
[task 2020-07-02T14:38:36.687Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 1 nsThread
[task 2020-07-02T14:38:36.687Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 3 nsURIHashKey
[task 2020-07-02T14:38:36.687Z] 14:38:36 INFO - TEST-INFO | leakcheck | tab leaked 2 nsWeakReference
[task 2020-07-02T14:38:36.687Z] 14:38:36 INFO - TEST-UNEXPECTED-FAIL | leakcheck | tab 12928 bytes leaked (BlobURL, BlobURLInputStream, CondVar, CookieJarSettings, CopyOnWriteValue, ...)
[task 2020-07-02T14:38:36.687Z] 14:38:36 INFO -
[task 2020-07-02T14:38:36.687Z] 14:38:36 INFO - runtests.py | Running tests: end.

Flags: needinfo?(ssengupta)

The win32 debug test failure seems to be caused by browser/components/attribution/test/browser/browser_AttributionCode_telemetry.js, when I disable this test, bc3 no longer fails for win32 debug.

Example test run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=578ffbb7dbae0af65b1b2164fb902187b47a46ee&selectedTaskRun=SjxpidHbTHq3u9woGQG76Q.0

When I run that test on a cross-compiled win32 debug build in a win64 environment, the test passes. It is not clear to be at all if this failure is related to my work. Is there something else I can try to reproduce this failure locally, so that I can try to fix it?

Flags: needinfo?(ssengupta) → needinfo?(ncsoregi)

On the try push it looks like it doesn't fail on bc3 anymore, but it still fails on bc2 as it's moving chunks (seen in the push which was backed out too).
Have you imported the patches on a try push and added all jobs?

Flags: needinfo?(ncsoregi)
Flags: needinfo?(ssengupta)

Instead of creating a new (non-private) window, the first existing window is used. Mime type added during blob creation. Other minor adjustments made.

Attachment #9166537 - Attachment description: Bug 1619953 - P10 - Test `browser_privatebrowsing_blobUrl.js` modified to pass on win32 r=baku → Bug 1619953 - P10 - Test `browser_privatebrowsing_blobUrl.js` modified to pass on win32 r=baku,mccr8

The test was found to fail for various builds in try runs for Bug 1656033. Running manually on the latest mozilla central at this time, the test did not pass for Linux and Windows 64 bit debug. This should be more closely investigated at a later time.

Filed Bug 1656033 to re-enable.

Depends on D85139

Flags: needinfo?(ssengupta)

The test was found to fail for various builds in try runs for Bug 1656033. Running manually on the latest mozilla central at this time, the test kept timing out for Linux and Windows 64 bit debug. This should be more closely investigated at a later time.

Filed Bug 1656510 to re-enable.

Depends on D85313

Attachment #9166537 - Attachment description: Bug 1619953 - P10 - Test `browser_privatebrowsing_blobUrl.js` modified to pass on win32 r=baku,mccr8 → Bug 1619953 - P10 - Test `browser_privatebrowsing_blobUrl.js` modified to pass on win32 r=baku
Attachment #9167382 - Attachment is obsolete: true
Pushed by abutkovits@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/18f5967c6264 P1 - Added missing include in PChromiumCDM.ipdl r=baku https://hg.mozilla.org/integration/autoland/rev/4023d64db104 P2 - Asynchronous BlobURLDataRequest introduced r=baku https://hg.mozilla.org/integration/autoland/rev/962b1016439e P3 - Asynchronous BlobURLInputStream added r=baku https://hg.mozilla.org/integration/autoland/rev/67ea7d621ba6 P4 - BlobURLChannel now creates BlobURLInputStream to acquire blob data r=baku https://hg.mozilla.org/integration/autoland/rev/e917c642bcd1 P5 - BlobURLChannel allows loading blob data that is very recently revoked r=baku https://hg.mozilla.org/integration/autoland/rev/e26c24b59a29 P6 - BlobURLChannel creates BlobURLInputStream only when content stream is opened r=baku https://hg.mozilla.org/integration/autoland/rev/258a4545fadc P7 - mozJSSubScriptLoader::ReadScript() now attempts reading from input stream even if content length is not already known r=baku https://hg.mozilla.org/integration/autoland/rev/bc827fb0f4de P8 - Test `browser_blob-channelname.js` converted to xpcshell-test and updated to work with asynchronous input streams r=baku,necko-reviewers https://hg.mozilla.org/integration/autoland/rev/1e1523043d23 P9 - Test `browser_ContentSearch.js` disabled for linux64debug when fission not enabled r=baku https://hg.mozilla.org/integration/autoland/rev/21c1e0210d45 P10 - Test `browser_privatebrowsing_blobUrl.js` modified to pass on win32 r=mccr8 https://hg.mozilla.org/integration/autoland/rev/82321c8cba45 P11 - Mochitest `test_presentation_dc_sender.html` disabled for e10s r=mccr8 https://hg.mozilla.org/integration/autoland/rev/5409c44e043f P12 - Blob data is retrieved locally for non-http/https URLs r=baku
Regressions: 1660636
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: