950308 - TextTrackList, TextTrackCue, TextTrack should probably BindToOwner themselves

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Boris Zbarsky [:bzbarsky]]

It looks like nsDOMEventTargetHelper subclasses that are bound to a window should tell the superclass that via BindToOwner.  TextTrackList doesn't do that.

Olli, how serious a problem is this?

Comment 1

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

It used to be a security issue, these days it is preventing certain CC/memory management optimizations.
Also, GetParentObject() returns null, except in this case where we store mGlobal and override
DETH's GetParentObject.

Comment 2

[Rick Eyre (:reyre)]

Attachment: v1: TextTrack, TextTrackCue, TextTrackList should BindToOwner r=smaug

This is a partial patch, haven't fixed TextTrackCue yet. I'd like to get some feedback to see if I'm heading in the right direction for this.

Also, I'm having a hard time figuring out how to implement this for TextTrackCue. TextTrackCue gets inited with a GlobalObject from the JS Bindings, but I'm not sure how to convert a GlobalObject to an nsPIDOMWindow object, which DOMEventTargetHelper needs for its ctor. Is there already a way to do this, or do I need to build something new?

Comment 3

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

nsCOMPtr<PIDOMWindow> w = do_QueryInterface(aGlobal.GetAsSupports());
should work.

Comment 4

[Rick Eyre (:reyre)]

Attachment: v2: TextTrack, TextTrackCue, TextTrackList should BindToOwner r=smaug

Thanks for the information Olli. This should be good for review.

Comment 5

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

Comment on attachment 8400912 [details] [diff] [review]
v2: TextTrack, TextTrackCue, TextTrackList should BindToOwner r=smaug

>-  mTrack = new TextTrack(OwnerDoc()->GetParentObject(), kind, label, srcLang,
>+  mTrack = new TextTrack(OwnerDoc()->GetWindow(), kind, label, srcLang,
You want GetInnerWindow

>+  nsPIDOMWindow* ownerWindow = mMediaElement->OwnerDoc()->GetWindow();
GetInnerWindow()

>+TextTrack::TextTrack(nsPIDOMWindow* aOwnerWindow,
>                      TextTrackKind aKind,
>                      const nsAString& aLabel,
>                      const nsAString& aLanguage,
>                      TextTrackMode aMode,
>                      TextTrackReadyState aReadyState,
>                      TextTrackSource aTextTrackSource)
>-  : mParent(aParent)
>+  : DOMEventTargetHelper(aOwnerWindow)

Did you actually test this patch in a debug build?
Since as far as I see, you'd get assertions when passing outer window to DETH constructor

Comment 6

[Rick Eyre (:reyre)]

Thanks for review.

(In reply to Olli Pettay [:smaug] from comment #5)

> Did you actually test this patch in a debug build?
> Since as far as I see, you'd get assertions when passing outer window to
> DETH constructor

Nope, not in a debug build.

Comment 7

[Rick Eyre (:reyre)]

Attachment: v3: TextTrack, TextTrackCue, TextTrackList should BindToOwner r=smaug

Made sure to test this one on a debug build.

Comment 8

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

Comment on attachment 8401055 [details] [diff] [review]
v3: TextTrack, TextTrackCue, TextTrackList should BindToOwner r=smaug

nsDOMEventTargetHelper(nsPIDOMWindow* aWindow) calls
SetIsDOMBinding() so you could remove those from your ctors.

>+TextTrackCue::TextTrackCue(nsPIDOMWindow* aOwnerWindow,
>                            double aStartTime,
>                            double aEndTime,
>                            const nsAString& aText,
>                            ErrorResult& aRv)
>-  : mText(aText)
>+  : DOMEventTargetHelper(aOwnerWindow)
>+  , mText(aText)
>   , mStartTime(aStartTime)
>   , mEndTime(aEndTime)
>   , mReset(false)
> {
>   SetDefaultCueSettings();
>-  MOZ_ASSERT(aGlobal);
>+  MOZ_ASSERT(aOwnerWindow);
Oh, now I realize... first), it is possible to hit this assertion because
element->OwnerDoc()->GetInnerWindow() may return null when the document has been unloaded, and
you access its elements from another window.
but second), how should this all work if one does
var a = document.implementation.createHTMLDocument().createElement("audio")
and then access the track stuff from 'a'?

For that case not using GetInnerWindow(), but GetScriptHandlingObject(bool& aHasHadScriptHandlingObject)
would be more consistent with other stuff.
You need to QI that return value to nsPIDOMWindow.
But the main question is that what should happen if that method returns null and aHasHadScriptHandlingObject becomes true.
Usually we don't let one to create an object in such case, see for example nsDocument::CloneDocHelper, and for consistency it would be good to do the same here too.
Sorry that I didn't think about this yesterday.

Otherwise this looks good.

Comment 9

[Rick Eyre (:reyre)]

(In reply to Olli Pettay [:smaug] from comment #8)

> Oh, now I realize... first), it is possible to hit this assertion because
> element->OwnerDoc()->GetInnerWindow() may return null when the document has
> been unloaded, and
> you access its elements from another window.
> but second), how should this all work if one does
> var a = document.implementation.createHTMLDocument().createElement("audio")
> and then access the track stuff from 'a'?

How do you detect that this is the case? Would it just be that GetInnerWindow() would return null and so we would use GetScriptHandlingObject() instead?

> Sorry that I didn't think about this yesterday.

No worries!

Comment 10

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

Use GetScriptHandlingObject(), and not GetInnerWindow()

Comment 11

[Rick Eyre (:reyre)]

Attachment: v4: TextTrack, TextTrackCue, TextTrackList should BindToOwner r=smaug

Hope this addresses the issues.

Comment 12

[Rick Eyre (:reyre)]

Attachment: v4: TextTrack, TextTrackCue, TextTrackList should BindToOwner r=smaug

Comment 13

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

Comment on attachment 8403338 [details] [diff] [review]
v4: TextTrack, TextTrackCue, TextTrackList should BindToOwner r=smaug

Because element.track may now return null, you need to make
it nullable in http://mxr.mozilla.org/mozilla-central/source/dom/webidl/HTMLTrackElement.webidl#29
And looks like we might access various member variables of 
TextTrackManager without checking whether they are null.
Somewhat annoying to fix, I know.

Comment 14

[Rick Eyre (:reyre)]

Attachment: v5: TextTrack, TextTrackCue, TextTrackList should BindToOwner r=smaug

Thanks for review Olli. I've made the Track attribute on the HTMLTrackElement IDL nullable and put in the pointer guards.

Comment 15

[Rick Eyre (:reyre)]

Attachment: v6: TextTrack, TextTrackCue, TextTrackList should BindToOwner r=smaug

Rebasing against m-c.

Try looks good: https://tbpl.mozilla.org/?tree=Try&rev=72007c6dcc9f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/351a8f7d3512

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/351a8f7d3512