1116382 - MediaSources leak until the document is released, due to URL.createObjectURL()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Karl Tomlinson (:karlt)]

createObjectURL(MediaSource) should revoke the URL after awaiting a stable state.

http://www.w3.org/TR/2014/CR-media-source-20140717/#widl-URL-createObjectURL-DOMString-MediaSource-mediaSource

"When this method is invoked, the user agent must run the following steps:

 1.  Return a unique MediaSource object URL that can be used to dereference the mediaSource argument, and run the rest of the algorithm asynchronously.

 2.  provide a stable state

 3.  Revoke the MediaSource object URL by calling revokeObjectURL() on it."

Comment 1

[Karl Tomlinson (:karlt)]

Blocking bug 1088553, because auto-revoking URLs could break sites if they start to depend on lack of revocation, by creating the URL and setting the src attribute in separate tasks, for example.

Comment 2

[Karl Tomlinson (:karlt)]

HTMLMediaElement will need some modification to avoid interpreting the URL
after it is revoked.  Currently setting the src attribute queues an event to
interpret the URL.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: webm/vorbis audio file for testcase

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: testcase

1. Set media.mediasource.enabled and media.mediasource.webm.enabled prefs to
   true, if not already.

2. Load testcase.

3. Observe memory use growing (in top or similar).

4. Load about:memory in another tab.

5. Click "Minimize Memory Use".

Expected: Memory usage reduces somewhat, before growing again.

Actual: Memory use continues to rise.

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #2)
> HTMLMediaElement will need some modification to avoid interpreting the URL
> after it is revoked.  Currently setting the src attribute queues an event to
> interpret the URL.

The situation is similar for object urls requiring explicit revocation,
because their async use makes it difficult for clients to know when it would
be safe to revoke, unless the url is resolution immediately after the client
provides, which is the approach used in [1] after much discussion.

HTMLMediaElement use is very asynchronous when there are other sources for the
media element that are tried first.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=17765

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: accept abstract strings in NewURIFromString()

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: resolve MediaSource urls on Source element src attributes when set

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: resolve MediaSource urls on media element src attributes when set

Element::SetAttr() looks designed to allow script to run when its
nsAutoScriptBlocker is destroyed.  I want the url to be resolved before script
runs and so using the existing SetAttr() override would not be suitable.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: reuse some UTF8toUTF16 code

Tidying up before moving this around.

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: always ShutdownDecoder() so that MediaSourceDecoder alone can manage MediaSource::Detach()

In a subsequent patch, mMediaSource will be closely associated with
mLoadingSrc and so may be set even when there is no MediaDecoder.

MediaSource::Attach()/Detach() will remain aligned with creation and shutdown
of the MediaSourceDecoder.

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: use MediaSource from resolution of MediaSource urls when set

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: no need to check IsMediaSourceURI(mLoadingSrc) once mMediaSource has been set

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: auto revoke MediaSource object URLs

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: adjust explict revoke test to fail if revocation fails

Previously this test would pass even without the revokeObjectURL().

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: test auto-revoking behavior with URL.createObjectURL(MediaSource)

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: test opening referenced MediaSource after URL.revokeObjectURL()

Comment 17

[Bobby Holley (:bholley)]

Comment on attachment 8612072 [details] [diff] [review]
use MediaSource from resolution of MediaSource urls when set

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

::: dom/html/HTMLMediaElement.cpp
@@ +2525,5 @@
> +          nsAutoString spec;
> +          GetCurrentSrc(spec);
> +          const char16_t* params[] = { spec.get() };
> +          ReportLoadError("MediaLoadInvalidURI", params, ArrayLength(params));
> +        }

Can you move this hunk to the patch in attachment #8611644 [details] [diff] [review]?

Comment 18

[Bobby Holley (:bholley)]

Comment on attachment 8612079 [details] [diff] [review]
auto revoke MediaSource object URLs

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

::: dom/base/URL.cpp
@@ +17,5 @@
>  #include "nsEscape.h"
>  #include "nsNetCID.h"
>  #include "nsNetUtil.h"
> +#include "nsWidgetsCID.h"
> +#include "nsIAppShell.h"

We really need to stop including all this widget junk everywhere just to do RunInStableState. Can you make a helper on nsContentUtils to abstract away the appshell piece? Followup is fine.

@@ +149,5 @@
> +    {
> +      nsHostObjectProtocolHandler::RemoveDataEntry(mUrl);
> +      return NS_OK;
> +    }
> +    nsAutoCString mUrl;

nsAutoCString shouldn't be used on the heap - use nsCString.

@@ +150,5 @@
> +      nsHostObjectProtocolHandler::RemoveDataEntry(mUrl);
> +      return NS_OK;
> +    }
> +    nsAutoCString mUrl;
> +  };

Also, seems like this could just be a lambda?

@@ +152,5 @@
> +    }
> +    nsAutoCString mUrl;
> +  };
> +
> +  nsRefPtr<RunnableRevoke> revocation = new RunnableRevoke;

Does the style guide say anything about whether we should use () here?

@@ +167,5 @@
> +  nsCOMPtr<nsIAppShell> appShell = do_GetService(kAppShellCID);
> +  rv = appShell->RunInStableState(revocation);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv), "RunInStableState() failure");
> +
> +  CopyASCIItoUTF16(revocation->mUrl, aResult);

Here's a sketch of my proposal for this method:

nsCString url;
nsresult rv =
  nsHostObjectProtocolHandler::AddDataEntry(NS_LITERAL_CSTRING(MEDIASOURCEURI_SCHEME), &aSource, principal, url);
  if (NS_FAILED(rv)) {
    aError.Throw(rv);
    return;
}

// The spec says that these URLs must be revoked in stable state.
nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction([url] () -> { nsHostObjectProtocolHandler::RemoveDataEntry(url); });
nsContentUtils::RunInStableState(r.forget());
CopyASCIItoUTF16(url, aResult);

Thoughts?

Comment 19

[Bobby Holley (:bholley)]

Comment on attachment 8612252 [details] [diff] [review]
test opening referenced MediaSource after URL.revokeObjectURL()

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

Beautiful work - thanks for the elegant micropatches Karl!

Comment 20

[Karl Tomlinson (:karlt)]

Attachment: auto revoke MediaSource object URLs

(In reply to Bobby Holley (:bholley) from comment #18)
> > +#include "nsWidgetsCID.h"
> > +#include "nsIAppShell.h"
> 
> We really need to stop including all this widget junk everywhere just to do
> RunInStableState. Can you make a helper on nsContentUtils to abstract away
> the appshell piece? Followup is fine.

Makes sense.  Bug 1171785.

> 
> @@ +149,5 @@
> > +    {
> > +      nsHostObjectProtocolHandler::RemoveDataEntry(mUrl);
> > +      return NS_OK;
> > +    }
> > +    nsAutoCString mUrl;
> 
> nsAutoCString shouldn't be used on the heap - use nsCString.

Hmmm.

"It is normally not a good idea to use this class on the heap, because it will
allocate space which may be wasted if the string it contains is significantly
smaller or any larger than 64 characters." [1]

Here we know that the string will fit within the 64 characters and this is a
short-lived object, so any waste of space seems insignificant compared to the
extra allocation required.

However, we are going to copy into what is probably an nsCString so yes we
might as well do it straight up and the runnable and result can share the same
buffer.

Also nsAutoCString would make a copy of a lambda closure [2] extra
unfortunate.

> > +  };
> 
> Also, seems like this could just be a lambda?

It can be.  I avoided NS_NewRunnableFunction because of the copying of the
lambda closure required, and I wasn't clear on whether nsCString would share
buffers.  Given nsCString will share buffers, I hope the copy is not so much
of an issue, but I expect the compiler will do little to optimize as the
functions are not inline.  Maybe the cost of copying the lambda closure is
worth taking to keep this tidy and easier to follow.

> > +  nsRefPtr<RunnableRevoke> revocation = new RunnableRevoke;
> 
> Does the style guide say anything about whether we should use () here?

I didn't find anything.  C++ is often quirky.  The zero-initialization is not
relevant here because the class provides its own constructor.  That leaves
opportunity to choose the style and () is kinda nice to indicate that a
constructor is run.  But we don't typically use () when constructing on the
stack.  nsCString url() is a function declaration, so perhaps that's a reason
to be inconsistent.

> @@ +167,5 @@
> > +  nsCOMPtr<nsIAppShell> appShell = do_GetService(kAppShellCID);
> > +  rv = appShell->RunInStableState(revocation);
> > +  MOZ_ASSERT(NS_SUCCEEDED(rv), "RunInStableState() failure");
> > +
> > +  CopyASCIItoUTF16(revocation->mUrl, aResult);
> 
> Here's a sketch of my proposal for this method:
> 
> nsCString url;

nsHostObjectProtocolHandler::GenerateURIString() builds the string piecemeal
[3] and nsTSubstring_CharT::MutatePrep() doesn't start with any minimum buffer
size, so I wondered whether this may be better with a an nsAutoCString.

But we are going to want to copy into nsCString anyway, so I'm going to claim
that GenerateURIString() should use SetCapacity() if perf is an issue, and so
nsCString seems good.

> Thoughts?

OK.

[1] https://hg.mozilla.org/mozilla-central/annotate/98820360ab66/xpcom/string/nsTString.h
[2] https://hg.mozilla.org/mozilla-central/annotate/98820360ab66/xpcom/glue/nsThreadUtils.h#l250
[3] https://hg.mozilla.org/mozilla-central/annotate/98820360ab66/dom/base/nsHostObjectProtocolHandler.cpp#l393

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd44786e85f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c077afa1fbf
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e1078de4590
https://hg.mozilla.org/integration/mozilla-inbound/rev/941944df6912
https://hg.mozilla.org/integration/mozilla-inbound/rev/990ac59aeaae
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd80106d7465
https://hg.mozilla.org/integration/mozilla-inbound/rev/76bc62c51e9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9117150ca7e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/94fcc63014b3

Comment 22

[Karl Tomlinson (:karlt)]

I pushed everything except the auto-revoking and its test, because the other changes make sense on their own, and so I don't need to rebase through log level changes again.

I'll look into bug 1171785 more next week.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bd44786e85f4
https://hg.mozilla.org/mozilla-central/rev/0c077afa1fbf
https://hg.mozilla.org/mozilla-central/rev/6e1078de4590
https://hg.mozilla.org/mozilla-central/rev/941944df6912
https://hg.mozilla.org/mozilla-central/rev/990ac59aeaae
https://hg.mozilla.org/mozilla-central/rev/dd80106d7465
https://hg.mozilla.org/mozilla-central/rev/76bc62c51e9e
https://hg.mozilla.org/mozilla-central/rev/9117150ca7e6
https://hg.mozilla.org/mozilla-central/rev/94fcc63014b3

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7af729730a83
https://hg.mozilla.org/integration/mozilla-inbound/rev/d71b2d0ce994

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7af729730a83
https://hg.mozilla.org/mozilla-central/rev/d71b2d0ce994