Closed Bug 1275449 Opened 4 years ago Closed 4 years ago

AnimationEffectTiming::SetEasing crash in page-mode addon context

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(2 files)

No description provided.
Looks like we fail to check the result of AnimationUtils::GetCurrentRealmDocument in:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/b535eb7300d8#l1.13

(I'd mark this as blocking bug 1244643 but I can't find out how to do that with the experimental Bugzilla interface.)
Blocks: 1244643
Comment on attachment 8757152 [details]
MozReview Request: Bug 1275449 - Part 1: AnimationEffectTiming::SetEasing crash in page-mode addon context. r=hiro

I believe hiro reviewed the original patch
Attachment #8757152 - Flags: review?(bbirtles) → review?(hiikezoe)
I wonder we can provide any test cases here?
I'd like to know the reason why there is no test before reviewing the fix.
Flags: needinfo?(daisuke)
Okay, I try to write page-mod addon test such as 
https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/addons
Also, I am thinking we don't need new tests for other context.

May we put the test to under dom/animation/tests/addons/?
Flags: needinfo?(daisuke)
Hi Hiro,
I will put the test to under addon-sdk/source/test/ since I could not test if dom/animation/test/addons.
I think we can create a test addon for the crash.  The addon just calls setEasing.  Can't we?
Thanks, Hiro.
Brian advised me. We may be able to refer devtool's test way in order to write the test more easily.
Once, I'll take a look at those tests.
Comment on attachment 8757152 [details]
MozReview Request: Bug 1275449 - Part 1: AnimationEffectTiming::SetEasing crash in page-mode addon context. r=hiro

https://reviewboard.mozilla.org/r/55686/#review52726

I think the null check should be done in AnimationEffectTiming::SetEasing().
But, as far as I'm concerned I don't think failing SetEasing in addon context is the right behavior.  Should we use the parent document of AnimationEffectTiming for parsing easing values?
Attachment #8757152 - Flags: review?(hiikezoe)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> Comment on attachment 8757152 [details]
> MozReview Request: Bug 1275449: AnimationEffectTiming::SetEasing crash in
> page-mode addon context. r?birtles
> 
> https://reviewboard.mozilla.org/r/55686/#review52726
> 
> I think the null check should be done in AnimationEffectTiming::SetEasing().
> But, as far as I'm concerned I don't think failing SetEasing in addon
> context is the right behavior.  Should we use the parent document of
> AnimationEffectTiming for parsing easing values?

I suggested that we could fix the crash in this bug (since it might need to be uplifted) then find a more general solution in a separate bug. There are a few other places where we throw if GetCurrentRealmDocument returns nullptr, so we should probably fix them all at once (and write tests too).
Hi Boris,

This bug is about a crash that occurs when we fail to check the result of AnimationUtils::GetCurrentRealmDocument.[1] That function appears to return nullptr in some contexts with plugins (particularly the page-mod plugin).

In particular Daisuke can reproduce the situation with code like the following:

> const contentScript = function() {
>   const div = document.createElement("div");
>   document.body.appendChild(div);
>   div.animate({ opacity: [0, 1] }, 100000 );
>   try {
>     document.getAnimations()[0].effect.timing.easing = "linear";
>     console.log("ok");
>   } catch(e) {
>     console.log(e);
>   }
> };
> const sandbox = new SpecialPowers.Cu.Sandbox(window);
> sandbox.importFunction(document, "document");
> sandbox.importFunction(console, "console");
> SpecialPowers.Cu.evalInSandbox(`(${contentScript.toSource()})()`, sandbox);

I'm really out of my depth as to what the different kinds of documents and contexts are but I'm wondering if:

a) There is a simpler way to reproduce this situation (so we can write a crashtest)
b) There is some other context/document we should be referring to here

The function that's triggering the crash in this case, SetEasing, wants a document since it wants to parse some CSS. It's not critical that this function work in this context, but if there's an easy way to get it to do so then that would be useful.

Thanks!

[1] https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/dom/animation/AnimationUtils.cpp#40
Flags: needinfo?(bzbarsky)
In the testcase shown in comment 5, the current realm is the sandbox.  This is not a window global, and does not have an associated document, obviously.

> a) There is a simpler way to reproduce this situation (so we can write a crashtest)

To reproduce this situation you need script in a non-window global (a sandbox, a JSM, etc) touching some API that uses AnimationUtils::GetCurrentRealmDocument via Xrays.  You should be able to write a chrome mochitest for this sort of thing; the given testcase is pretty good for that.

> b) There is some other context/document we should be referring to here

No idea.  It depends on what behavior the API that uses AnimationUtils::GetCurrentRealmDocument wants.

In general, it's not clear to me that the current realm is the right thing to use at _any_ of the callsites of AnimationUtils::GetCurrentRealmDocument.  As I recall, when that code was checked in the idea was that it was mostly a placeholder and the real desired behavior was yet to be figured out.

For the particular case of SetEasing, what should happen when the setter from one document is called on a timing object from another document?  Which document should be used for parsing the CSS?  Are timing objects always associated with documents?

I recommend reading https://html.spec.whatwg.org/multipage/webappapis.html#concept-entry-everything for a description of how the different globals an algorithm like this might care about are defined (entry, incumbent, current, relevant); that might help with the being out of your depth.  If it doesn't, please feel free to ping me and we can talk about it!
Flags: needinfo?(bzbarsky)
> I recommend reading
> https://html.spec.whatwg.org/multipage/webappapis.html#concept-entry-
> everything for a description of how the different globals an algorithm like
> this might care about are defined (entry, incumbent, current, relevant);
> that might help with the being out of your depth.  If it doesn't, please
> feel free to ping me and we can talk about it!

Thanks! That's really helpful.

> In general, it's not clear to me that the current realm is the right thing
> to use at _any_ of the callsites of AnimationUtils::GetCurrentRealmDocument.
> As I recall, when that code was checked in the idea was that it was mostly a
> placeholder and the real desired behavior was yet to be figured out.
> 
> For the particular case of SetEasing, what should happen when the setter
> from one document is called on a timing object from another document?  Which
> document should be used for parsing the CSS?  Are timing objects always
> associated with documents?

That's a really good question and something that needs to be defined.  There's an open spec issue for that partly covers this[1] but I haven't tackled yet because I was waiting to see if Anne would add some blanket definitions to WebIDL that might help with this (and because I wasn't confident I understood the subtle differences in definitions here) but maybe I should just go ahead and add something.

At a hunch, I think it might make sense to associate timing objects with documents. It seems like that would fix bugs like this one, in particular.

So,

* 'new AnimationEffectTiming(ReadOnly)', once that is spec'ed, would
  presumably be associated with the document on the global that defines
  AnimationEffectTiming(ReadOnly). i.e. window.document for 'new
  AnimationEffectTiming' but frames[0].document for 'new
  frames[0].AnimationEffectTiming'.

* For 'new Animation' and 'new KeyframeEffect(ReadOnly)' (which eventually
  ultimately create a timing object), we'd associate the timing object
  with the document on the global that defines
  Animation/KeyframeEffect(ReadOnly).

* We still need to define what global to use for Element.animate() (I
  guess we should use the relevant realm for Element) but presumably
  once we work that out, we'll be able to use the document from the
  corresponding global.

  As a result, suppose we define a constructor for AnimationEffect and
  in future we support timing functions defined by a url() parameter,
  then I guess we'd have a situation like:

    // Resolves URL against frames[0].document.baseURI
    const timing = new frames[0].AnimationEffectTiming(
                     { easing: 'url(easings.html#bounce)' });

    const div = querySelector('div');
    // Resolves URL against document.baseURI
    const animation = div.animate({ ... },
                                  { easing: 'url(easings.html#swing)' });
    animation.effect.timing = timing;

    // *Still* resolves URL against frames[0].document.baseURI
    // (i.e. we don't re-associate the timing object when associating
    //  it with animation from the parent document)
    timing.easing = 'url(easing.html#spin)';

There are a few unusual things:

* It's possible to specify 'easing' on keyframes as well as timing
  objects. Once we introduce a constructor for AnimationEffectTiming, it
  will be possible to create a situation where the following resolve
  using different documents:

    anim.effect.setKeyframes({ opacity: 0, easing: 'url(...)' }, ...);
    anim.effect.timing.easing = 'url(...)';

* url() references in keyframe *values* will still be resolved using the
  composed document of the elements they are actually being applied to
  (as part of computing values).

  i.e.

    anim.effect.setKeyframes({ filter: 'url(...)' }, ...);

  will always resolve against the composed document of
  anim.effect.target (and is updated if anim.effect.target is modified).

In terms of implementation, I've gone through all the call sites of GetCurrentRealmDocument below and basically it's used:

i.   for supplying context used when parsing CSS
ii.  as the parent object to matching WebIDL objects to window objects
iii. for looking up the style context (this seems to be a bug)

So, if associated timing objects with documents makes sense, then I suppose we could store the associated document as the parent object in (ii). Then for (i) we can use the mDocument pointer.

(iii) seems like a bug that we should just fix.

I'll add the above discussion to the spec issue[1] and we can continue it there.

[1] https://github.com/w3c/web-animations/issues/145

Call sites of GetCurrentRealmDocument and what they use the returned document for:

* AnimationEffectTiming::SetEasing
  - to pass to ParseEasing (which uses the CSS parser)
* KeyframeEffectReadOnly::SetKeyframes
  - so it can get the corresponding nsStyleContext via its PresShell and use
    that to ultimately resolve computed values. That seems wrong. It should be
    using the composed doc of the target element and if there's no target
    element it doesn't need to resolve computed values.
* KeyframeEffectReadOnly::ConstructKeyframeEffect
  - to pass to ParseEasing (which uses the CSS parser)
  - to pass to the AnimationEffectReadOnly base class ctor where it is stored as
    the parent object
  - to pass to the AnimationEffectTimingReadOnly class ctor where it is stored
    as the parent object
* KeyframeUtils::GetKeyframesFromObject
  - to pass to RequiresAdditiveAnimation which uses the CSS parser to check if
    we have valid properties. This is only a temporary method until we actually
    support additive animation so I'm less concerned about this case.
* ConvertKeyframeSequence in KeyframeUtils.cpp
  - to initialize a CSSParser from the document's CSSLoader
  - to pass to ParseEasing (which uses the CSS parser)
* GetKeyframeListFromPropertyIndexedKeyframe in KeyframeUtils.cpp
  - to pass to ParseEasing (which uses the CSS parser)
  - to initialize a CSSParser from the document's CSSLoader
  - to pass to MakePropertyValuePair which pulls various properties off it to
    pass to the passed-in CSSParser object
> At a hunch, I think it might make sense to associate timing objects with documents.

To be clear, in the spec it MUST already be associated with documents, or more precisely with windows.  All non-worker globals on the web have windows/documents involved.

There's some question about whether to associate with a document or a window that only affects initial about:blank and document.open situations, of course.

My question about whether timing objects are always associated with documents was a question about our implementation, where we _do_ have all sorts of non-document globals around.

> 'new AnimationEffectTiming(ReadOnly)', once that is spec'ed, would
> presumably be associated with the document on the global that defines
> AnimationEffectTiming(ReadOnly).

This is already somewhat covered by the IDL spec.  'new AnimationEffectTiming' creates an object associated with the window the AnimationEffectTiming constructor came from.  That object could snapshot a document association at creation time, or it could reget the docuemnt via window.document, as you say.  Which one you pick depends on how you want initial about:blank to behave.  Similar for anything else created via a constructor.

>* We still need to define what global to use for Element.animate() (I
>  guess we should use the relevant realm for Element) but presumably
>  once we work that out, we'll be able to use the document from the
>  corresponding global.

What should happen for elements in documents that are not directly window-associated (e.g. XHR or DOMParser result documents)?

>  then I guess we'd have a situation like:

This seems reasonable.

>* url() references in keyframe *values* will still be resolved using the
>  composed document of the elements they are actually being applied to
>  (as part of computing values).

Computing or parsing?  The CSS parser converts url() values to absolute URLs at parse time, using the base URL it's initialized with.
(In reply to Boris Zbarsky [:bz] from comment #17)
> > At a hunch, I think it might make sense to associate timing objects with documents.
> 
> To be clear, in the spec it MUST already be associated with documents, or
> more precisely with windows.  All non-worker globals on the web have
> windows/documents involved.

Ok, that makes sense.

> There's some question about whether to associate with a document or a window
> that only affects initial about:blank and document.open situations, of
> course.

I don't understand the implications of the about:blank/document.open case but it doesn't sound like we need any special handling for that here.

> My question about whether timing objects are always associated with
> documents was a question about our implementation, where we _do_ have all
> sorts of non-document globals around.

Ok. Assuming we can find a suitable definition for what document to use for all cases (e.g. by using the relevant realm for Element.animate if that makes sense), and assuming we snapshot that association by storing that document as the parent object then it sounds like we could address the case where this API is used in a non-document global context. But that assumes we can find a suitable document and I'm not sure what the answer is to that for resource documents as you point out below.

> > 'new AnimationEffectTiming(ReadOnly)', once that is spec'ed, would
> > presumably be associated with the document on the global that defines
> > AnimationEffectTiming(ReadOnly).
> 
> This is already somewhat covered by the IDL spec.  'new
> AnimationEffectTiming' creates an object associated with the window the
> AnimationEffectTiming constructor came from.  That object could snapshot a
> document association at creation time, or it could reget the docuemnt via
> window.document, as you say.  Which one you pick depends on how you want
> initial about:blank to behave.  Similar for anything else created via a
> constructor.

Snapshotting at creation time seems simplest to me but I don't know how that fits with the rest of the platform.

> >* We still need to define what global to use for Element.animate() (I
> >  guess we should use the relevant realm for Element) but presumably
> >  once we work that out, we'll be able to use the document from the
> >  corresponding global.
> 
> What should happen for elements in documents that are not directly
> window-associated (e.g. XHR or DOMParser result documents)?

I'm really not familiar with that situation. Is there somewhere I can read up on that? e.g. I wonder would making Element.animate() throw for those cases be reasonable?

> >* url() references in keyframe *values* will still be resolved using the
> >  composed document of the elements they are actually being applied to
> >  (as part of computing values).
> 
> Computing or parsing?  The CSS parser converts url() values to absolute URLs
> at parse time, using the base URL it's initialized with.

Oh, good point. We will parse them into nsCSSValue objects when they are set, and compute them into StyleAnimationValue objects whenever the target element changes. So for parsing I guess we should use the same document we use for parsing timing properties on the object.
> I don't understand the implications of the about:blank/document.open case

Briefly, those are the only two cases in which windows and documents are not 1-1.  In the initial about:blank case, navigation to a same-origin thing from it creates a new document but reuses the same window.  In the document.open() case, the same document sticks around but a new window is created.

> but I don't know how that fits with the rest of the platform

It varies.  Some things are associated with a window (typically things that are gotten off it directly).  Some things are tied to a particular document (most obviously, nodes).

> Is there somewhere I can read up on that?

Both XHR and DOMParser can take an input string, parse it, and return the resulting document.  These documents are not rendered, but I believe they _can_ have inline styles (which don't affect anything other than getComputedStyle).  A simple testcase:

  var doc = new DOMParser().parseFromString("<div>", "text/html");

> I wonder would making Element.animate() throw for those cases be reasonable?

It might be, yes.
Review commit: https://reviewboard.mozilla.org/r/56814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56814/
Attachment #8757152 - Attachment description: MozReview Request: Bug 1275449: AnimationEffectTiming::SetEasing crash in page-mode addon context. r?birtles → MozReview Request: Bug 1275449 - Part 1: AnimationEffectTiming::SetEasing crash in page-mode addon context. r?hiro
Attachment #8758560 - Flags: review?(hiikezoe)
Attachment #8757152 - Flags: review?(hiikezoe)
Comment on attachment 8757152 [details]
MozReview Request: Bug 1275449 - Part 1: AnimationEffectTiming::SetEasing crash in page-mode addon context. r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55686/diff/1-2/
Attachment #8757152 - Flags: review?(hiikezoe)
Comment on attachment 8757152 [details]
MozReview Request: Bug 1275449 - Part 1: AnimationEffectTiming::SetEasing crash in page-mode addon context. r=hiro

https://reviewboard.mozilla.org/r/55686/#review53520

I still think that the check should be done in SetEasing().  I mean that we should check the return value of AnimationUtils::GetCurrentRealmDocument() in SetEasing() just like other callers of AnimationUtils::GetCurrentRealmDocument() do.  Is there any strong reason you don't do that?
Thanks Hiro! I'll do that.
Comment on attachment 8757152 [details]
MozReview Request: Bug 1275449 - Part 1: AnimationEffectTiming::SetEasing crash in page-mode addon context. r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55686/diff/2-3/
Attachment #8757152 - Flags: review?(hiikezoe)
Comment on attachment 8758560 [details]
MozReview Request: Bug 1275449 - Part 2: Add a test. r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56814/diff/1-2/
Comment on attachment 8758560 [details]
MozReview Request: Bug 1275449 - Part 2: Add a test. r=hiro

https://reviewboard.mozilla.org/r/56814/#review53526

I think we need to set dom.animations-api.core.enabled if this test is not run in chrome priviledge.

::: dom/animation/test/mochitest.ini:44
(Diff revision 1)
>    mozilla/file_disabled_properties.html
>    mozilla/file_hide_and_show.html
>    mozilla/file_partial_keyframes.html
>    style/file_animation-seeking-with-current-time.html
>    style/file_animation-seeking-with-start-time.html
> +  sandbox/set-easing.html

Where is this file?

::: dom/animation/test/sandbox/test_set-easing.html:14
(Diff revision 1)
> +<body>
> +
> +<script>
> +"use strict";
> +
> +SimpleTest.waitForExplicitFinish();

You don't need to call waitForExplicitFinish() because add_task() calls it.

::: dom/animation/test/sandbox/test_set-easing.html:30
(Diff revision 1)
> +    doesThrow(() => {
> +      document.getAnimations()[0].effect.timing.easing = "linear";
> +    }, "AnimationTimingFunction::SetEasing should throw in sandbox.");

I am just curious that mochitest can show/track down to this line when it fails.

::: dom/animation/test/sandbox/test_set-easing.html:35
(Diff revision 1)
> +  doesNotThrow(() => {
> +    document.getAnimations()[0].effect.timing.easing = "linear";
> +  }, "AnimationTimingFunction::SetEasing should not throw in normal context.");

I don't think doesNotThrow() is needed here because mochitest will fail if there are unexpected exceptions.  Also this check is really necessary?
Attachment #8758560 - Flags: review?(hiikezoe) → review+
Attachment #8757152 - Flags: review?(hiikezoe) → review+
Comment on attachment 8757152 [details]
MozReview Request: Bug 1275449 - Part 1: AnimationEffectTiming::SetEasing crash in page-mode addon context. r=hiro

https://reviewboard.mozilla.org/r/55686/#review53540
Thank you for your reviewing!
Comment on attachment 8757152 [details]
MozReview Request: Bug 1275449 - Part 1: AnimationEffectTiming::SetEasing crash in page-mode addon context. r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55686/diff/3-4/
Attachment #8757152 - Attachment description: MozReview Request: Bug 1275449 - Part 1: AnimationEffectTiming::SetEasing crash in page-mode addon context. r?hiro → MozReview Request: Bug 1275449 - Part 1: AnimationEffectTiming::SetEasing crash in page-mode addon context. r=hiro
Attachment #8758560 - Attachment description: MozReview Request: Bug 1275449 - Part 2: Add tests. r?hiro → MozReview Request: Bug 1275449 - Part 2: Add a test. r=hiro
Comment on attachment 8758560 [details]
MozReview Request: Bug 1275449 - Part 2: Add a test. r=hiro

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/56814/diff/2-3/
Assignee: nobody → daisuke
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6cde20f6294
Part 1: AnimationEffectTiming::SetEasing crash in page-mode addon context. r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/f746d2a857b8
Part 2: Add a test. r=hiro
Keywords: checkin-needed
I filed bug 1277456 for fixing all the uses of GetCurrentRealmDocument discussed in comment 15 onwards.
https://hg.mozilla.org/mozilla-central/rev/f6cde20f6294
https://hg.mozilla.org/mozilla-central/rev/f746d2a857b8
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.