1279186 - Blob URLs are not shared between processes

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P2)

Description

[Geoff Lankow (:darktrojan)]

Attachment: blob.js

If a Blob is created in a content process, then loaded in a new tab, the contents of the new tab cannot be saved. These messages appear in the console:

> [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIWebBrowserPersist.savePrivacyAwareURI]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 571"  data: no]
> [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIAnnotationService.setPageAnnotation]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: resource://app/modules/DownloadsCommon.jsm :: onDownloadChanged :: line 781"  data: no]

I've attached some code to be run in the Scratchpad (browser context). It captures the current page on a canvas, creates a Blob from the canvas and loads it in a new tab. Trying to save the image from the new tab fails.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

You are sending the createObjectURL() from a child process to a parent process?  I don't think this will work since the life of the blob URL is tied to the global in the child.

Can you just send the blob directly?  That seems to be supported?

https://dxr.mozilla.org/mozilla-central/source/dom/ipc/tests/test_blob_sliced_from_child_process.html#38

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: blob.html

> You are sending the createObjectURL() from a child process to a parent process?  I don't think this will work since the life of the blob URL is tied to the global in the child.

I only did it that way to tell the chrome to open a new tab. If I send the blob I can't create the URL and open a new tab because the content process doesn't know about the URL.

Anyway that's a little irrelevant - I've attached a much simpler script which doesn't involve chrome privileges, and fails the same way.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Andrea, what do you think?

Comment 4

[Andrea Marchesini [:baku]]

The problem here is that the blob URLs are generated and stored in the current process. And for e10s that means: content process.
But the saving of the content runs in the parent process.

We have to centralize the storing of blob URLs not only for this reason: soon we will have multiple content processes and blob URLs should work in any of these processes.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: blob.patch

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Andrea Marchesini (:baku) from comment #4)
> soon we will have multiple content processes and blob URLs should work in
> any of these processes.
Why?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

hmm,  I guess shared worker between two processes.

Comment 9

[Andrea Marchesini [:baku]]

Or BroadcastChannel... etc.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8764684 [details] [diff] [review]
blob.patch

Perhaps khuey could review this.

Comment 11

[Gabor Krizsanits (INACTIVE)]

Kyle, do you mind if I steal this review from you? I've been looking into it and I think I can handle it.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

By all means, go for it.

Comment 13

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8764684 [details] [diff] [review]
blob.patch

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

I think this is the right approach. I still have some questions so cancelling the r? for now. If the sync messages turn out to be problematic we can think about a caching mechanism for optimize it later. I would love to have a test for this that covers the multiple content process scenarios, but that can be a follow up as we discussed. I'm concerned about the part where we send a bunch of sync messages from the GC, we should avoid that somehow... for that you probably need another hashmap on parent side that maps entries to globals. I'm not sure how big of an issue is this in practice... maybe this can be also a follow up if it's a rare scenario that a global has many bloburls and the implementation turns out to be troublesome.

There are a few nits, and there is one thing I don't quite get how to handle right. If a content process crashes what happens with its entries on parent side. What will happen then if someone from another process is trying to use one of those urls?

::: dom/base/nsHostObjectProtocolHandler.cpp
@@ +314,5 @@
>      initialized = true;
>      RegisterStrongMemoryReporter(new mozilla::HostObjectURLsReporter());
> +
> +    if (XRE_IsParentProcess()) {
> +      RegisterStrongMemoryReporter(new mozilla::BlobURLsReporter());

I'm not sure this is right. Wouldn't this mean that we report the blob in the parent process while its data actually lives in a content process instead.

@@ +336,5 @@
> +  nsCOMPtr<BlobImpl> blob = do_QueryInterface(aObject);
> +  if (blob && !XRE_IsParentProcess()) {
> +    MOZ_ASSERT(aScheme.EqualsLiteral(BLOBURI_SCHEME));
> +    ContentChild* cc = ContentChild::GetSingleton();
> +    if (NS_WARN_IF(!cc)) {

I think if we ever get here we can just crash, so I'm fine with using ContentChild::GetSingleton() unguarded.

@@ +380,1 @@
>  nsHostObjectProtocolHandler::RemoveDataEntry(const nsACString& aUri)

Yeah I think we really need that nsIGlobalObject::UnlinkHostObjectURIs so we don't end up sending sync messages in loop when the GC hits.

@@ +447,5 @@
>    return NS_OK;
>  }
>  
> +static bool
> +GetDataInfo(const nsACString& aUri, DataInfo& aInfo)

I don't see the point of changing the return value here... Is there a case where we differentiate between returning true or false when the |aInfo| is not set? If not then leaving it as it was would simplify this patch.

::: dom/base/nsHostObjectProtocolHandler.h
@@ +55,5 @@
>                                 nsISupports* aObject,
>                                 nsIPrincipal* aPrincipal,
>                                 nsACString& aUri);
>    static void RemoveDataEntry(const nsACString& aUri);
> +  static bool GetDataEntryInfo(const nsACString& aUri,

Instead of the two ** arguments here would it make sense to use one DataInfo& ?

::: dom/ipc/ContentParent.cpp
@@ +5857,5 @@
> +                                   const IPC::Principal& aPrincipal,
> +                                   nsCString* aUri,
> +                                   nsresult* aRv)
> +{
> +  if (!aBlobParent) {

how can this be null? shouldn't we just assert here?

::: dom/ipc/PContent.ipdl
@@ +269,5 @@
>    FMRadioRequestSeekParams;
>    FMRadioRequestCancelSeekParams;
>  };
>  
> +struct GetBlobURLInfoResultSuccess

I don't understand why are you using Get in the name of these structs.

@@ +275,5 @@
> +  PBlob blob;
> +  Principal principal;
> +};
> +
> +struct GetBlobURLInfoResultFailure

I think instead of this you could use void_t in the next union (BlobURLInfoResult) like the optional arguments in ipdl. And then we would have one less structs here...

::: dom/ipc/PermissionMessageUtils.h
@@ +27,5 @@
>    {}
>  
>    operator nsIPrincipal*() const { return mPrincipal.get(); }
>  
> +  Principal& operator=(const Principal& aOther)

I think you won't need this with the changes around the IPC structs. The explicit ctor should be enough.

Comment 14

[Gabor Krizsanits (INACTIVE)]

Actually... can we get away with doing the removing part when the GC hits with async messages? That would be the simplest to implement.

Comment 15

[Andrea Marchesini [:baku]]

> There are a few nits, and there is one thing I don't quite get how to handle
> right. If a content process crashes what happens with its entries on parent
> side. What will happen then if someone from another process is trying to use
> one of those urls?

Yes! This is important. I'll write a separate patch, same bug.

> I'm not sure this is right. Wouldn't this mean that we report the blob in
> the parent process while its data actually lives in a content process
> instead.

Interesting, there is a bug there: bug 1285508.
The problem is that, with this setup, we don't store blobURL/BlobImpl in the content process.
The report must be where the hashtable is.

> I don't see the point of changing the return value here... Is there a case
> where we differentiate between returning true or false when the |aInfo| is
> not set? If not then leaving it as it was would simplify this patch.

This is used in many places. aInfo is struct in the heap, initialized only if the entry is found in gHashTable.

> Instead of the two ** arguments here would it make sense to use one
> DataInfo& ?

I don't want to expose DataInfo. DataInfo so far is internal only struct.

> > +  if (!aBlobParent) {
> 
> how can this be null? shouldn't we just assert here?

What about if the content process has been hacked?

Comment 16

[Andrea Marchesini [:baku]]

Attachment: blob_multi_e10s.patch

Comment 17

[Andrea Marchesini [:baku]]

Attachment: blob_multi_e10s_b.patch - part 2

This second patch is about child process crashing.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: blob_multi_e10s_b.patch - part 2

hg qref.

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to Andrea Marchesini (:baku) from comment #15)
> > I don't see the point of changing the return value here... Is there a case
> > where we differentiate between returning true or false when the |aInfo| is
> > not set? If not then leaving it as it was would simplify this patch.
> 
> This is used in many places. aInfo is struct in the heap, initialized only
> if the entry is found in gHashTable.

I don't understand your point. This function used to return a nullptr if the entry was not found, we could just return a nullptr if something else goes wrong and not change the function signature. I don't see any code in this patch that would have problem with this approach and it made the code simpler. So why did you decide to change the function signature? I mean, which case are you trying to guard against exactly?

> I don't want to expose DataInfo. DataInfo so far is internal only struct.

Alright.

> 
> > > +  if (!aBlobParent) {
> > 
> > how can this be null? shouldn't we just assert here?
> 
> What about if the content process has been hacked?

What about it? I mean if someone takes control over the content process and sets aBlobParent to point to something else that's a problem ofc but we cannot guard against that anyway. This check won't guard against that except if one sets it to null, then we crash with a nullptr which is a pretty harmless way of crashing. 

But what I meant is that I think IPC would crash before calling this function with a null aBlobParent.

Comment 20

[Andrea Marchesini [:baku]]

> I don't understand your point. This function used to return a nullptr if the
> entry was not found, we could just return a nullptr if something else goes
> wrong and not change the function signature. I don't see any code in this
> patch that would have problem with this approach and it made the code
> simpler. So why did you decide to change the function signature? I mean,
> which case are you trying to guard against exactly?

This function was returning a pointer of a DataInfo object contained in the gDataTable. If the entry was not contained in gDataTable, it was returning a nullptr. Now we have to deal with IPC so I don't have anything keeping alive a DataInfo struct. This is the reason why I changed it to have a reference. In the IPC scenario I do:

+    aInfo.mObject = static_cast<BlobChild*>(success.blobChild())->GetBlobImpl();
+    aInfo.mPrincipal = success.principal();
+    return true;

For non e10s I do:

+  aInfo = *res;
+  return true;

Where *res is taken from the gDataTable.

I don't see how I can keep the previous signature.

Comment 21

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8769151 [details] [diff] [review]
blob_multi_e10s.patch

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

(In reply to Andrea Marchesini (:baku) from comment #20)
> This function was returning a pointer of a DataInfo object contained in the
> gDataTable.

I'm sorry, I must have been tired :)

::: dom/ipc/PContent.ipdl
@@ +269,5 @@
>    FMRadioRequestSeekParams;
>    FMRadioRequestCancelSeekParams;
>  };
>  
> +struct BlobURLInfoResultSuccess

Not sure if we still need Success in the name, but I don't feel very strongly against it either...

Comment 22

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8769154 [details] [diff] [review]
blob_multi_e10s_b.patch - part 2

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

Looks great.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 1 - BlobURL broadcasted

1. ContentChild must retrieve the list of blobURLs when initialized
2. Any Register/Unregister is broadcasted to any child

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 2 - Contentparent unregistes BlobURLs if the child crashes

Same patch, but built on top of the new setup

Comment 25

[Gabor Krizsanits (INACTIVE)]

Hey Bill, this problem turned out to be a bit of a headache and I was wondering if you could give us your opinion on it.

The idea was to register the URLs in the chrome process, and then code from child processes would fetch everything from there with sync messages. Not a perfect design but because of the sync API it's hard to do anything better. Later on my plan was to do some caching to avoid the sync messages when possible.

Anyway. As it turned out there is a problem with this approach:
1:40 PM <baku> The issue is this, sometimes the Blob is created in the parent process and then sent to the child process.
1:40 PM <baku> example: FilePicker.
1:41 PM <baku> the FilePicker runs in the parent process. always
1:41 PM <baku> then we send the Blob back to the content process
1:41 PM <baku> and often, this is for a <input type="file" />
1:41 PM <baku> now, if you want to create a BlobURL for that blob, in the child process
1:41 PM <baku> when we send the Blob to the parent for the registration (with my patch)
1:42 PM <baku> we are smart enough to say: "ah! this is not a remote blob! We know this blob, it's a blob created in the parent process and we have the original in a table. Let's use the original one."
1:42 PM <baku> so we use the 'original' blob, created by the FilePicker.
1:42 PM <gabor> so far so good
1:42 PM <baku> we have it, becuase the RemoteBlobImpl keeps alive the 'original' blob in the parent process.
1:42 PM <baku> great!
1:43 PM <baku> now... we do an XHR in the child process using the BlobURL.
1:43 PM <baku> we create a nsIChannel
1:43 PM <baku> this is sync necko operation
1:43 PM <baku> and we call ::NewChannel2
1:43 PM <baku> this calls GetDataInfo
1:43 PM <baku> we end up calling SendGetDataInfo
1:43 PM <baku> the sync method that my patch introduces.
1:44 PM <baku> in the parent process we want to send back the blob to the child process.
1:44 PM <baku> so we create a BlobParent actor, we register a new entry in the sIDTable (the table for the lookup for the original vs remote blob)
1:45 PM <baku> the BlobParent::Create calls SendPBlobConstructor
1:45 PM <baku> async operation.
1:45 PM <baku> this is done in order to create a BlobChild in the child process.
1:45 PM <baku> but... we are in a sync method so that constructor message is not received.
1:45 PM <baku> or well, it's received, but after the SendGetDataInfo
1:46 PM <baku> so, the blob in the child is unknown, and everything fails.

Andrea's idea to use broadcasting for the registration. It would probably fix the problem (except a few edge cases probably where it can get racy) but I wish there were something else we could do...

What do you think? (also once we have a final patch can I flag you for sr?)

Comment 26

[Andrea Marchesini [:baku]]

Attachment: part 1 - BlobURL broadcasted

In the meantime I updated the patch.

Comment 27

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

> 1:45 PM <baku> but... we are in a sync method so that constructor message is not received.
> 1:45 PM <baku> or well, it's received, but after the SendGetDataInfo

Yeah, this is a pretty annoying problem with IPDL. We could run the constructor during the sync message processing, but that would probably break a bunch of things that don't expect that right now.

However, the broadcasting approach seems like it should work. How would it be racy?

Comment 28

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8770450 [details] [diff] [review]
part 1 - BlobURL broadcasted

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

Thanks, this looks great. And sorry for the lag with the review.

::: dom/base/nsHostObjectProtocolHandler.cpp
@@ +75,5 @@
>  // Memory reporting for the hash table.
>  namespace mozilla {
>  
> +void
> +MaybeBroadcastBlobURLRegistration(const nsACString& aURI,

Uber NIT: I think I would prefer to remove the Maybe from the name of this function and do the blobImpl check before call. Both in the child and parent process case it will be broadcast eventually, IF the object is a blob and there are no fatal errors. We can check the if it's a blob part before the call and if we care to handle the errors the function could return false in case of error.

Anyway, it's just a personal preference, so please consider it and if you like your version better I'm totally fine with it.

::: dom/ipc/ContentChild.cpp
@@ +2650,5 @@
> +  for (uint32_t i = 0; i < aRegistrations.Length(); ++i) {
> +    BlobURLRegistrationData& registration = aRegistrations[i];
> +    RefPtr<BlobImpl> blobImpl =
> +      static_cast<BlobChild*>(registration.blobChild())->GetBlobImpl();
> +    if (blobImpl) {

I think this could be an assert.

::: dom/ipc/ContentParent.cpp
@@ +2513,5 @@
>    }
>  #endif
>  
> +  {
> +    RefPtr<ServiceWorkerRegistrar> swr = ServiceWorkerRegistrar::Get();

I don't quite get why you need this extra {} scope here around this block.

@@ +2522,5 @@
> +    Unused << SendInitServiceWorkers(ServiceWorkerConfiguration(registrations));
> +  }
> +
> +  {
> +    nsTArray<BlobURLRegistrationData> registrations;

And here.

Comment 29

[Gabor Krizsanits (INACTIVE)]

(In reply to Bill McCloskey (:billm) from comment #27)
> > 1:45 PM <baku> but... we are in a sync method so that constructor message is not received.
> > 1:45 PM <baku> or well, it's received, but after the SendGetDataInfo
> 
> Yeah, this is a pretty annoying problem with IPDL. We could run the
> constructor during the sync message processing, but that would probably
> break a bunch of things that don't expect that right now.
> 
> However, the broadcasting approach seems like it should work. How would it
> be racy?

Yeah, after thinking it through again and reading the spec a bit more I think it's
fine. I just needed an extra confirmation that we are doing the right thing here,
and there aren't any shortcuts I'm missing. I think I worried a bit too much about
broadcasting but I realized if it's only per process and not per frame broadcast then
it should be fine.

About the racy part, I don't think there is any use case we should be worried about.
I'm sure it's observable what we do here from an add-on, if someone really wants it
(two same origin tab from different content process, registering a Blob URI from one
and quickly trying to remove it from the other), but I don't think there is a legitimate
use case where this could be an issue. Thanks for looking at it though.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/effbc83efa08
Blob URLs in multi-e10s - part 1 - BlobURLs broadcasted, r=gabor
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e641cbce05
Blob URLs in multi-e10s - part 2 - Contentparent unregistes BlobURLs if the child crashes, r=gabor

Comment 31

[Iris Hsiao [:ihsiao]]

Hi, Andrea, sorry had to back out for the Mochitest M(1) leak issue. e.g: https://treeherder.mozilla.org/logviewer.html#?job_id=31929256&repo=mozilla-inbound#L19160

Comment 32

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbaa820b8c7c
Backed out changeset f9e641cbce05 for Mochitest M(1) leak issue.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bda4f49e0f5
Backed out changeset effbc83efa08

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d086a8f4846f
Blob URLs in multi-e10s - part 1 - BlobURLs broadcasted, r=gabor
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f9aefe6b8fa
Blob URLs in multi-e10s - part 2 - Contentparent unregistes BlobURLs if the child crashes, r=gabor
https://hg.mozilla.org/integration/mozilla-inbound/rev/317caf83210f
Blob URLs in multi-e10s - part 3 - Remove all the blobURLs when the child goes away, r=me

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d086a8f4846f
https://hg.mozilla.org/mozilla-central/rev/6f9aefe6b8fa
https://hg.mozilla.org/mozilla-central/rev/317caf83210f

Comment 37

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking as fix-optional for 49, as I don't think this is critical to uplift.

Comment 38

[Marco Castelluccio [:marco]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #37)
> Marking as fix-optional for 49, as I don't think this is critical to uplift.

Note that if we uplift this, we should also uplift the fix for bug 1288997.