Closed Bug 1116382 Opened 5 years ago Closed 4 years ago

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

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla41

People

(Reporter: karlt, Assigned: karlt)

References

(Blocks 1 open bug)

Details

Attachments

(13 files, 1 obsolete file)

9.61 KB, audio/webm
Details
623 bytes, text/html
Details
1.87 KB, patch
bholley
: review+
Details | Diff | Splinter Review
4.94 KB, patch
bholley
: review+
Details | Diff | Splinter Review
5.68 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.82 KB, patch
bholley
: review+
Details | Diff | Splinter Review
3.47 KB, patch
bholley
: review+
Details | Diff | Splinter Review
6.38 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.06 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.80 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.82 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.13 KB, patch
bholley
: review+
Details | Diff | Splinter Review
1.71 KB, patch
bholley
: review+
Details | Diff | Splinter Review
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."
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.
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.
Summary: MediaSources leak until the document is released → MediaSources leak until the document is released, due to URL.createObjectURL()
Blocks: ship-MSE
Attached file 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.
Depends on: 1114885
Priority: P2 → P1
(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
Attachment #8611641 - Flags: review?(bobbyholley)
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Attachment #8611641 - Flags: review?(bobbyholley) → review+
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.
Attachment #8611644 - Flags: review?(bobbyholley)
Tidying up before moving this around.
Attachment #8611646 - Flags: review?(bobbyholley)
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.
Attachment #8612071 - Flags: review?(bobbyholley)
Attachment #8612079 - Flags: review?(bobbyholley)
Previously this test would pass even without the revokeObjectURL().
Attachment #8612250 - Flags: review?(bobbyholley)
Attachment #8611642 - Flags: review?(bobbyholley) → review+
Attachment #8611644 - Flags: review?(bobbyholley) → review+
Attachment #8611646 - Flags: review?(bobbyholley) → review+
Attachment #8612071 - Flags: review?(bobbyholley) → review+
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]?
Attachment #8612072 - Flags: review?(bobbyholley) → review+
Attachment #8612073 - Flags: review?(bobbyholley) → review+
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?
Attachment #8612079 - Flags: review?(bobbyholley) → review+
Attachment #8612250 - Flags: review?(bobbyholley) → review+
Attachment #8612251 - Flags: review?(bobbyholley) → review+
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!
Attachment #8612252 - Flags: review?(bobbyholley) → review+
Depends on: 1171785
(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
Attachment #8615792 - Flags: review?(bobbyholley)
Attachment #8612079 - Attachment is obsolete: true
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.
Keywords: leave-open
Attachment #8615792 - Flags: review?(bobbyholley) → review+
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.