Use blob URLs exclusively rather than having mediastream and mediasource URLs

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: annevk, Assigned: baku)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
See https://github.com/whatwg/url/issues/126 for details.

I think the basic idea is to have one store for all these object URLs and one scheme.
I think baku knows about this situation.
Flags: needinfo?(amarchesini)
(Assignee)

Comment 2

2 years ago
I wrote something for blobURL in multi-e10s (bug 1279186) and I want to see that landed before changing that code again: with this patch we have 2 indexes,  one on the parent process just for blob URLs, and another one 'locally' (per process) for any other mediastream/mediasource/font URLs. The solution here is to have 2 indexes again, but both for blob URLs: if the underlying object is a real blob/file object, we use the index running in the parent, otherwise the local (per process) one.
Assignee: nobody → amarchesini
Depends on: 1279186
(Assignee)

Comment 3

a year ago
Created attachment 8772915 [details] [diff] [review]
patch

It can be that I have to fix some tests. The patch has been pushed to try.
Flags: needinfo?(amarchesini)
Attachment #8772915 - Flags: review?(bugs)
Comment on attachment 8772915 [details] [diff] [review]
patch

>+  nsresult rv =
>+    GenerateURIString(NS_LITERAL_CSTRING(BLOBURI_SCHEME), aPrincipal, aUri);
Might be worth to add a helper method GenerateBlobURIString which takes the latter 2 params and passes 
NS_LITERAL_CSTRING(BLOBURI_SCHEME) to GenerateURIString

>@@ -448,22 +496,22 @@ nsHostObjectProtocolHandler::GetAllBlobU
>   if (!gDataTable) {
>     return true;
>   }
> 
>   for (auto iter = gDataTable->ConstIter(); !iter.Done(); iter.Next()) {
>     DataInfo* info = iter.UserData();
>     MOZ_ASSERT(info);
> 
>-    nsCOMPtr<BlobImpl> blobImpl = do_QueryInterface(info->mObject);
>-    if (!blobImpl) {
>+    if (info->mObjectType == DataInfo::eBlobImpl) {
>       continue;
>     }
This looks like a bug. The old version continues if we don't have blobImpl, the new one only if we do have it.


> 
>-    PBlobParent* blobParent = aCP->GetOrCreateActorForBlobImpl(blobImpl);
>+    MOZ_ASSERT(info->mBlobImpl);
And this assert then fires.

>+  ErrorResult rv;
>+
> #ifdef DEBUG
>   nsCString spec;
>-  uri->GetSpec(spec);
>+  rv = uri->GetSpec(spec);
>+  if (NS_WARN_IF(rv.Failed())) {
>+    return rv.StealNSResult();
>+  }
Weird to use ErrorResult for nsresult handling. Please use nsresult.


> nsresult
> NS_GetStreamForMediaStreamURI(nsIURI* aURI, mozilla::DOMMediaStream** aStream)
> {
>   NS_ASSERTION(IsMediaStreamURI(aURI), "Only call this with mediastream URIs");
> 
>-  nsISupports* dataObject = GetDataObject(aURI);
>-  if (!dataObject) {
>+  nsCString spec;
>+  nsresult rv = aURI->GetSpec(spec);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  DataInfo* info = GetDataInfo(spec);
There should be a version of GetDataInfo which takes nsIURI*
That way there would be less copy-pasting code
Attachment #8772915 - Flags: review?(bugs) → review-
(Assignee)

Comment 5

a year ago
Created attachment 8773182 [details] [diff] [review]
blobUrl2.patch
Attachment #8772915 - Attachment is obsolete: true
Attachment #8773182 - Flags: review?(bugs)
Attachment #8773182 - Flags: review?(bugs) → review+

Comment 6

a year ago
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e4999d9dd2
Use blob URLs exclusively rather than having mediastream and mediasource URLs, r=smaug
Depends on: 1288499

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d6e4999d9dd2
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Duplicate of this bug: 1198553
You need to log in before you can comment on or make changes to this bug.