Status

()

enhancement
P2
normal
Last year
4 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

(Blocks 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 8 obsolete attachments)

11.56 KB, patch
smaug
: review-
Details | Diff | Splinter Review
11.57 KB, patch
smaug
: review-
Details | Diff | Splinter Review
7.89 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.66 KB, patch
smaug
: review-
Details | Diff | Splinter Review
3.34 KB, patch
smaug
: review-
Details | Diff | Splinter Review
12.01 KB, patch
smaug
: review+
Details | Diff | Splinter Review
33.85 KB, patch
smaug
: review-
Details | Diff | Splinter Review
Assignee

Description

Last year
BlobURLs are currently broadcasted to any process. This is expensive from a memory point of view, and it requires several IPC calls each time a blobURL is created or revoked.

This bug is about removing the blobURL broadcasting.
Assignee

Comment 1

Last year
I'm going to ask for a review when the dependency bug is landed.
Priority: -- → P2
Assignee

Comment 5

Last year
Posted patch part 5 - tests (obsolete) — Splinter Review
Assignee

Updated

Last year
Blocks: 1438214
Assignee

Comment 7

10 months ago
Attachment #8988715 - Attachment is obsolete: true
Attachment #9004513 - Flags: review?(bugs)
Assignee

Comment 8

10 months ago
Attachment #8988716 - Attachment is obsolete: true
Attachment #9004514 - Flags: review?(bugs)
Assignee

Comment 9

10 months ago
Attachment #8988717 - Attachment is obsolete: true
Attachment #9004515 - Flags: review?(bugs)
Assignee

Comment 10

10 months ago
Attachment #8989135 - Attachment is obsolete: true
Attachment #9004516 - Flags: review?(bugs)
Assignee

Comment 11

10 months ago
Attachment #9004518 - Flags: review?(bugs)
Assignee

Comment 12

10 months ago
Attachment #9004519 - Flags: review?(bugs)
Assignee

Comment 13

10 months ago
Attachment #9004515 - Attachment is obsolete: true
Attachment #9004515 - Flags: review?(bugs)
Attachment #9004683 - Flags: review?(bugs)
Assignee

Comment 14

10 months ago
Attachment #9004516 - Attachment is obsolete: true
Attachment #9004516 - Flags: review?(bugs)
Attachment #9004684 - Flags: review?(bugs)
Assignee

Comment 15

10 months ago
Fully green on try.
Attachment #9004684 - Attachment is obsolete: true
Attachment #9004684 - Flags: review?(bugs)
Attachment #9004690 - Flags: review?(bugs)
Comment on attachment 9004519 [details] [diff] [review]
part 7 - documentation

>+/**
>+ * BlobURLs
>+ * ~~~~~~~~
>+ *
>+ * A blobURL is a URL connected to a DOM Blob content. It can be created and
>+ * used by any process. Each blobURL is registered in a hashtable in each
>+ * process,
really, each process? I thought the whole point was to avoid that.




>+ * BlobURLs and e10s
>+ * ~~~~~~~~~~~~~~~~~
>+ *
>+ * Another possible scenario is that, the current process wants to open an
>+ * unknown blobURL.
What is the other scenario? Something above? This isn't quite clear.




>+ * When a blobURL needs to be used, if it's unknown, a BlobURLChannel is
>+ * created, and this creates a BlobURLInputStream, which sends an IPC message to
>+ * the parent asking for that particular blobURL, as string
>+ * (ContentChild::RetrieveBlobURL)
>+ *
>+ * If the parent process known this blobURL, asynchornsly,
s/knows/

>+ it sends an IPC
s/asynchornsly/asynchronously/ but the sentence is a bit odd. asynchornsly seems to refer to knowning about the bloburl, not about
sending it. Please rephrase.



>+ * message to the content process, passing the BlobImpl and its nsIPrincipal
>+ * (ContentParent::SendBlobURLReady)
>+ *
>+ * When BlobURLInputStream receives these objects, it reads data from the
>+ * BlobImpl's nsIInputStream when needed. In the meantime, BlobURLInputStream
>+ * behaves as a nsIAsyncInputStream.
'In the meantime' seems to be useless part of the sentence. Or does BlobURLInputStream behave as
nsIAsyncInputStream only for some time?


>+ * When a blobURL, marked as revoked, needs to be opened, we return an error.
>+ * Otherwise, we continue as described previously.
I don't understand this explanation. So, revoked blobURL can't be opened, but what does 
"otherwise" refer to here?
Attachment #9004519 - Flags: review?(bugs) → review-
Comment on attachment 9004518 [details] [diff] [review]
part 6 - tests

ok, this contains more than just tests.

We need to somehow ensure the blobs are in different processes. And need to have a test for revoking in one process and trying to use in another.
Attachment #9004518 - Flags: review?(bugs) → review-

Updated

10 months ago
Attachment #9004514 - Flags: review?(bugs) → review+
Comment on attachment 9004511 [details] [diff] [review]
part 1 - Introduce BlobURLProtocolHandler::GetBlobURLPrincipalOrOwnerURI

I don't understand the behavior of the new method. It returns blob url's principal or a null principal, but url is always just the blob url's principal's url.
and owner URI is returned even if blob url was revoked.


Rather odd inconsistency, and changes for example the behavior of NS_SecurityCompareURIs.
Attachment #9004511 - Flags: review?(bugs) → review-
Comment on attachment 9004513 [details] [diff] [review]
part 2 - Retrieve the principal from BlobURLChannel

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># Parent  42eef1146aac56d92671881798f1062dff18b9f7
>
>diff --git a/caps/NullPrincipalURI.cpp b/caps/NullPrincipalURI.cpp
>--- a/caps/NullPrincipalURI.cpp
>+++ b/caps/NullPrincipalURI.cpp
>@@ -27,17 +27,16 @@ NullPrincipalURI::NullPrincipalURI()
> NullPrincipalURI::NullPrincipalURI(const NullPrincipalURI& aOther)
> {
>   mPath.Assign(aOther.mPath);
> }
> 
> nsresult
> NullPrincipalURI::Init()
> {
>-  // FIXME: bug 327161 -- make sure the uuid generator is reseeding-resistant.
>   nsCOMPtr<nsIUUIDGenerator> uuidgen = services::GetUUIDGenerator();
>   NS_ENSURE_TRUE(uuidgen, NS_ERROR_NOT_AVAILABLE);
> 
>   nsID id;
>   nsresult rv = uuidgen->GenerateUUIDInPlace(&id);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   mPath.SetLength(NSID_LENGTH - 1); // -1 because NSID_LENGTH counts the '\0'
>diff --git a/caps/nsScriptSecurityManager.cpp b/caps/nsScriptSecurityManager.cpp
>--- a/caps/nsScriptSecurityManager.cpp
>+++ b/caps/nsScriptSecurityManager.cpp
>@@ -53,16 +53,17 @@
> #include "nsIURIFixup.h"
> #include "nsCDefaultURIFixup.h"
> #include "nsIChromeRegistry.h"
> #include "nsIResProtocolHandler.h"
> #include "nsIContentSecurityPolicy.h"
> #include "nsIAsyncVerifyRedirectCallback.h"
> #include "mozilla/Preferences.h"
> #include "mozilla/dom/BindingUtils.h"
>+#include "mozilla/dom/BlobURLChannel.h"
> #include "mozilla/NullPrincipal.h"
> #include <stdint.h>
> #include "mozilla/dom/ScriptSettings.h"
> #include "mozilla/ClearOnShutdown.h"
> #include "mozilla/StaticPtr.h"
> #include "nsContentUtils.h"
> #include "nsJSUtils.h"
> #include "nsILoadInfo.h"
>@@ -341,22 +342,22 @@ nsScriptSecurityManager::GetChannelResul
>   if (owner) {
>     CallQueryInterface(owner, aPrincipal);
>     if (*aPrincipal) {
>       return NS_OK;
>     }
>   }
> 
>   if (loadInfo) {
>-        if (!aIgnoreSandboxing && loadInfo->GetLoadingSandboxed()) {
>-          MOZ_ALWAYS_TRUE(NS_SUCCEEDED(loadInfo->GetSandboxedLoadingPrincipal(aPrincipal)));
>-          MOZ_ASSERT(*aPrincipal);
>-          InheritAndSetCSPOnPrincipalIfNeeded(aChannel, *aPrincipal);
>-          return NS_OK;
>-        }
>+    if (!aIgnoreSandboxing && loadInfo->GetLoadingSandboxed()) {
>+      MOZ_ALWAYS_TRUE(NS_SUCCEEDED(loadInfo->GetSandboxedLoadingPrincipal(aPrincipal)));
>+      MOZ_ASSERT(*aPrincipal);
>+      InheritAndSetCSPOnPrincipalIfNeeded(aChannel, *aPrincipal);
>+      return NS_OK;
>+    }
> 
>     bool forceInherit = loadInfo->GetForceInheritPrincipal();
>     if (aIgnoreSandboxing && !forceInherit) {
>       // Check if SEC_FORCE_INHERIT_PRINCIPAL was dropped because of
>       // sandboxing:
>       if (loadInfo->GetLoadingSandboxed() &&
>         loadInfo->GetForceInheritPrincipalDropped()) {
>         forceInherit = true;
>@@ -389,16 +390,26 @@ nsScriptSecurityManager::GetChannelResul
>                                                         uri,
>                                                         inheritForAboutBlank,
>                                                         false)) {
>         principalToInherit.forget(aPrincipal);
>         return NS_OK;
>       }
>     }
>   }
>+
>+  nsCOMPtr<nsIBlobURLChannel> blobURLChannel = do_QueryInterface(aChannel);
>+  if (blobURLChannel) {
>+    nsCOMPtr<nsIPrincipal> principal = blobURLChannel->GetPrincipal();
>+    if (principal) {
>+      principal.forget(aPrincipal);
>+      return NS_OK;
>+    }
>+  }
>+
>   nsresult rv = GetChannelURIPrincipal(aChannel, aPrincipal);
>   NS_ENSURE_SUCCESS(rv, rv);
>   InheritAndSetCSPOnPrincipalIfNeeded(aChannel, *aPrincipal);
>   return NS_OK;
> }
> 
> /* The principal of the URI that this channel is loading. This is never
>  * affected by things like sandboxed loads, or loads where we forcefully
>diff --git a/dom/file/uri/BlobURLChannel.cpp b/dom/file/uri/BlobURLChannel.cpp
>--- a/dom/file/uri/BlobURLChannel.cpp
>+++ b/dom/file/uri/BlobURLChannel.cpp
>@@ -4,16 +4,18 @@
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> #include "BlobURLChannel.h"
> #include "mozilla/dom/BlobImpl.h"
> 
> using namespace mozilla::dom;
> 
>+NS_IMPL_ISUPPORTS_INHERITED(BlobURLChannel, nsBaseChannel, nsIBlobURLChannel)
>+
> BlobURLChannel::BlobURLChannel(nsIURI* aURI,
>                                nsILoadInfo* aLoadInfo)
>   : mInitialized(false)
> {
>   SetURI(aURI);
>   SetOriginalURI(aURI);
>   SetLoadInfo(aLoadInfo);
> 
>@@ -30,17 +32,17 @@ void
> BlobURLChannel::InitFailed()
> {
>   MOZ_ASSERT(!mInitialized);
>   MOZ_ASSERT(!mInputStream);
>   mInitialized = true;
> }
> 
> void
>-BlobURLChannel::Initialize(BlobImpl* aBlobImpl)
>+BlobURLChannel::Initialize(BlobImpl* aBlobImpl, nsIPrincipal* aPrincipal)
> {
>   MOZ_ASSERT(!mInitialized);
> 
>   nsAutoString contentType;
>   aBlobImpl->GetType(contentType);
>   SetContentType(NS_ConvertUTF16toUTF8(contentType));
> 
>   if (aBlobImpl->IsFile()) {
>@@ -60,16 +62,18 @@ BlobURLChannel::Initialize(BlobImpl* aBl
> 
>   aBlobImpl->CreateInputStream(getter_AddRefs(mInputStream), rv);
>   if (NS_WARN_IF(rv.Failed())) {
>     InitFailed();
>     return;
>   }
> 
>   MOZ_ASSERT(mInputStream);
>+
>+  mPrincipal = aPrincipal;
>   mInitialized = true;
> }
> 
> nsresult
> BlobURLChannel::OpenContentStream(bool aAsync, nsIInputStream** aResult,
>                                   nsIChannel** aChannel)
> {
>   MOZ_ASSERT(mInitialized);
>@@ -85,9 +89,16 @@ BlobURLChannel::OpenContentStream(bool a
> 
>   return NS_OK;
> }
> 
> void
> BlobURLChannel::OnChannelDone()
> {
>   mInputStream = nullptr;
>+  mPrincipal = nullptr;
> }
>+
>+nsIPrincipal*
>+BlobURLChannel::GetPrincipal() const
>+{
>+  return mPrincipal;
>+}
>diff --git a/dom/file/uri/BlobURLChannel.h b/dom/file/uri/BlobURLChannel.h
>--- a/dom/file/uri/BlobURLChannel.h
>+++ b/dom/file/uri/BlobURLChannel.h
>@@ -13,40 +13,65 @@
> 
> class nsIURI;
> 
> namespace mozilla {
> namespace dom {
> 
> class BlobImpl;
> 
>-class BlobURLChannel final : public nsBaseChannel
>+#define BLOBURLCHANNEL_IID \
>+  { 0x128959c3, 0xf00d, 0x436b, \
>+    { 0xa2, 0xba, 0x4b, 0xd5, 0x45, 0x59, 0x04, 0x69 } }
>+
>+class nsIBlobURLChannel : public nsISupports
> {
> public:
>+  NS_DECLARE_STATIC_IID_ACCESSOR(BLOBURLCHANNEL_IID)
>+
>+  virtual nsIPrincipal*
>+  GetPrincipal() const = 0;
>+};
I don't see reason for nsIBlobURLChannel. BlobURLChannel could have IID itself, similar to dom::Element and such


> 
>+  // The principal of the Blob's parent.
parent? What is Blob's parent?

>+  nsCOMPtr<nsIPrincipal> principal;
>+
>+  // For revoked blobURLs, we use a null principal.
>+  if (blobURL->Revoked()) {
>+    principal = NullPrincipal::CreateWithoutOriginAttributes();
>+  }
>+
>+  // If it's a known blobURL, let's use the known principal.
>+  if (!principal) {
>+    DataInfo* info = GetDataInfoFromURI(aURI, true /*aAlsoIfRevoked */);
>+    if (info) {
>+      // We know that this is something else.
>+      if (info->mObjectType != DataInfo::eBlobImpl || !info->mBlobImpl) {
>+        return false;
>+      }
>+
>+      principal = info->mPrincipal;
>+    }
>+  }
>+
>+  if (principal) {
>+    if (aOwnerURI) {
>+      rv = principal->GetURI(aOwnerURI);
So here ownerURI can be also the null principal uri, but in the part 1 it wouldn't be.


>-  if (aPrincipal) {
>-    principal.forget(aPrincipal);
>+  // 'null/<uuid>' blobURLs should not leave the owning process.
>+  if (StringBeginsWith(path, NS_LITERAL_CSTRING("null/"))) {
>+    return false;
Why we do string comparison and not check whether it is a null principal?
Attachment #9004513 - Flags: review?(bugs) → review-

Updated

10 months ago
Attachment #9004683 - Flags: review?(bugs) → review+
Comment on attachment 9004690 [details] [diff] [review]
part 5 - Retrieve blobURL from the parent process

>+void
>+BlobURLChannel::InitializeRemote()
>+{
>+  MOZ_ASSERT(!XRE_IsParentProcess());
>+
>+  nsCOMPtr<nsIURI> uri;
>+  nsresult rv = GetURI(getter_AddRefs(uri));
>+  if (NS_WARN_IF(NS_FAILED(rv))) {
>+    InitFailed();
>+    return;
>+  }
>+
>+  RefPtr<BlobURL> blobURL;
>+  rv = uri->QueryInterface(kHOSTOBJECTURICID, getter_AddRefs(blobURL));
Huh, why is CID use for this. Could you file a followup to make BlobURL to have a normal IID

>+void
>+BlobURLChannel::SetPrincipal(nsIPrincipal* aPrincipal)
>+{
>+  MOZ_ASSERT(aPrincipal && !mPrincipal);
>+  MOZ_ASSERT_IF(mLoadInfo &&
>+                  !nsContentUtils::IsSystemPrincipal(mLoadInfo->LoadingPrincipal()),
>+                 ChromeUtils::IsOriginAttributesEqualIgnoringFPD(mLoadInfo->GetOriginAttributes(),
>+                                                                 BasePrincipal::Cast(aPrincipal)->OriginAttributesRef()));
Why ignoring FPD?


>+void
>+BlobURLInputStream::DataReceived(BlobImpl* aBlobImpl,
>+                                 nsIPrincipal* aPrincipal)
DataReceived sounds a bit weird, when we receive BlobImpl.  And then looks like this can be called with null params.
Could this be something like... UpdateStreamContent? Not too good name either though.


>+  if (hasAsyncWaitCallback) {
>+    if (mInputStream) {
>+      mInputStream->AsyncWait(this, asyncWaitFlags, asyncWaitRequestedCount,
>+                              asyncWaitTarget);
>+    } else {
>+      RefPtr<BlobURLInputStream> self = this;
>+      nsCOMPtr<nsIRunnable> r =
>+        NS_NewRunnableFunction("BlobURLInputStream::DataReceived", [self]() {
>+          nsCOMPtr<nsIInputStreamCallback> callback;
>+          {
>+            MutexAutoLock lock(self->mMutex);
>+            callback = self->mAsyncWaitCallback;
>+          }
>+
>+          if (callback) {
>+            callback->OnInputStreamReady(self);
>+          }
>+        });
>+
>+      if (asyncWaitTarget) {
>+        asyncWaitTarget->Dispatch(r, NS_DISPATCH_NORMAL);
>+      } else {
>+        r->Run();
>+      }
>+    }
>+  }
>+}
This needs some comment what is happening. What is the runnable function doing and why?


>+class BlobURLInputStream final : public nsIAsyncInputStream
>+                               , public nsIInputStreamLength
>+                               , public nsIAsyncInputStreamLength
>+                               , public nsIInputStreamCallback
>+{
>+public:
>+  NS_DECL_THREADSAFE_ISUPPORTS
>+  NS_DECL_NSIINPUTSTREAM
>+  NS_DECL_NSIASYNCINPUTSTREAM
>+  NS_DECL_NSIINPUTSTREAMLENGTH
>+  NS_DECL_NSIASYNCINPUTSTREAMLENGTH
>+  NS_DECL_NSIINPUTSTREAMCALLBACK
>+
>+  static already_AddRefed<nsIInputStream>
>+  Create(BlobURLChannel* aChannel, BlobURL* aURI);
>+
>+  void
>+  DataReceived(BlobImpl* aBlobImpl,
>+               nsIPrincipal* aPrincipal);
>+
>+private:
>+  explicit BlobURLInputStream(BlobURLChannel* aChannel);
>+  ~BlobURLInputStream();
>+
>+  Mutex mMutex;
What all is protected by this mutex. Needs some documentation.

>+
>+  enum State {
{ goes to its own line.

>+    ePending,
>+    ePendingClosed,
>+    eReady,
>+    eError,
>+  };
These states need documentation.



>@@ -836,16 +844,19 @@ private:
>   nsCOMPtr<nsIFile> mProfileDir;
> #endif
> 
>   // Hashtable to keep track of the pending GetFilesHelper objects.
>   // This GetFilesHelperChild objects are removed when RecvGetFilesResponse is
>   // received.
>   nsRefPtrHashtable<nsIDHashKey, GetFilesHelperChild> mGetFilesPendingRequests;
> 
>+  // Hashtable to keep track of the pending BlobURLInputStream objects.
>+  nsRefPtrHashtable<nsIDHashKey, BlobURLInputStream> mBlobURLPendingStreams;
Perhaps mPendingBlobURLStreams would sound a tad better.




I'll need to read this several times. And we need quite a few tests for all this stuff.
Attachment #9004690 - Flags: review?(bugs) → review-
Duplicate of this bug: 1513132

Comment 22

7 months ago
This is actually a fission blocker because it's about proactively broadcast data; rather than a rogue content process requesting it.
Blocks: fission
No longer blocks: fission-ipc
baku, are you planning on picking this back up? We're seeing crashes related to it over in bug 1513096 and it's definitely important for the content memshrink project.
Flags: needinfo?(amarchesini)
Assignee

Comment 24

6 months ago
This issue will be resolved indirectly by fission project. We currently broadcast blobURLs, because, a process can load them at any time. When we load them, we need to extract the owning principal synchronously (See principal CheckMayLoad and subsumes methods).

But, after fission, we don't need to broadcast them anymore because only the owning origin is allowed to open its own blobURLs and, all of this will be running on the same process.

farre, Is this correct?
Flags: needinfo?(amarchesini) → needinfo?(afarre)
Assignee

Updated

6 months ago
Duplicate of this bug: 1501003

That should be the case.

Flags: needinfo?(afarre)
You need to log in before you can comment on or make changes to this bug.