1423098 - Remove support for SMIL's accessKey feature

Status: RESOLVED FIXED

(Core :: SVG, enhancement)

Description

[Brian Birtles (:birtles)]

No other browser implements it and it's been the source of security-related bugs in the past (e.g. bug 704482).

Comment 1

[Brian Birtles (:birtles)]

Intent to unship: https://groups.google.com/forum/#!topic/mozilla.dev.platform/skw-Yj_Pdjk

Comment 2

[Daniel Holbert [:dholbert]]

--> Assigning to birtles, since I think he's intending to work on this per the wording in his intent-to-unship.

Comment 3

[Brian Birtles (:birtles)]

Yes, thanks. Sorry, I was unwell last week so didn't get to it. I'll try to get to it on the plane today.

Comment 4

[Brian Birtles (:birtles)]

Attachment: Bug 1423098 - Ignore invalid time value specifications rather than failing;

Review commit: https://reviewboard.mozilla.org/r/207068/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207068/

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8936351 [details]
Bug 1423098 - Ignore invalid time value specifications rather than failing;

https://reviewboard.mozilla.org/r/207068/#review212938

Thanks! This looks great.

r- for the time being, only because I think we might want to test one more thing (see below), which might want a quick review pass.

::: dom/smil/nsSMILTimeValueSpecParams.h:60
(Diff revision 1)
> -  // The repeat iteration (type=REPEAT) or access key (type=ACCESSKEY) to
> -  // respond to.
> -  uint32_t          mRepeatIterationOrAccessKey;
> +  // The repeat iteration to respond to.
> +  // Only used for REPEAT types.
> +  uint32_t mRepeatIteration;

The phrase "REPEAT types" sounds a bit confusing here, since mType has only one (not plural) repeat-flavored value.

Maybe:
 "Only used when mType=REPEAT."
?

::: dom/smil/test/test_smilAccessKey.xhtml:20
(Diff revision 1)
>  </svg>
>  </div>
>  <pre id="test">
>  <script class="testbody" type="text/javascript">
>  <![CDATA[
>  /** Test for SMIL accessKey support **/

s/support/lack-of-support/, perhaps?

::: dom/smil/test/test_smilAccessKey.xhtml:25
(Diff revision 1)
>  function main()
>  {
> -  gSvg.pauseAnimations();
> -
> +  testBeginValueIsNotSupported('accessKey(a)');
> +  testBeginValueIsNotSupported('accesskey(a)');

It's probably worth testing a list-valued begin value as well -- something like the following:

  begin="3s; accessKey(a)"
  begin="accessKey(a); 1s"

(Are the 3s and 1s values still expected to work there, or will we reject the whole attribute-parse operation?)

Comment 6

[Kohei]

Posted the site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2017/smil-accesskey-support-has-been-removed/

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 8936351 [details]
Bug 1423098 - Ignore invalid time value specifications rather than failing;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207068/diff/1-2/

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 8936351 [details]
Bug 1423098 - Ignore invalid time value specifications rather than failing;

https://reviewboard.mozilla.org/r/207068/#review213050

Can you explain the changes in this update? It's not clear to me why this affects the behavior. It looks like the code changes here are just adjusting how we report failure from SetBeginOrEndSpec, but it looks like we still end up reporting "some error code" in exactly the same conditions that we did before, so it's not clear to me why this produces a behavior change.  Do we react to different error codes differently, up the stack somewhere?

::: dom/smil/nsSMILTimedElement.cpp:1327
(Diff revision 2)
> -  nsresult rv = NS_OK;
> -  while (tokenizer.hasMoreTokens() && NS_SUCCEEDED(rv)) {
> +  bool hadFailure;
> +  while (tokenizer.hasMoreTokens()) {
>      nsAutoPtr<nsSMILTimeValueSpec>
>        spec(new nsSMILTimeValueSpec(*this, aIsBegin));
> -    rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
> +    nsresult rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
>      if (NS_SUCCEEDED(rv)) {
>        timeSpecsList.AppendElement(spec.forget());
> +    } else {
> +      hadFailure = true;
>      }
>    }
>  
> -  if (NS_FAILED(rv)) {
> -    ClearSpecs(timeSpecsList, instances, aRemove);
> -  }
> -
> +  // The return value from this is only used to determine if we should print
> +  // a console message or not, so we return failure if we had one or more
> +  // failures.
> +  return hadFailure ? NS_ERROR_FAILURE : NS_OK;

Two things

(1) I think you need to initialize hadFailure to false (optimistically) on the line where you declare it, right?

(Nothing ever sets it to false right now; and it looks like there are cases where we leave it uninitialized right now.)

(2) I didn't initially get the gist of the comment at the end. I think you should mention "so we don't bother distinguishing between different error-flavored nsresult values" or something along those lines.

::: dom/smil/test/test_smilTiming.xhtml:135
(Diff revision 2)
> -  testCases.push(StartTimeTest('3;;', 'none'));
> -  testCases.push(StartTimeTest('3;; ', 'none'));
> -  testCases.push(StartTimeTest(';3', 'none'));
> -  testCases.push(StartTimeTest(' ;3', 'none'));
> +  testCases.push(StartTimeTest('3;;', 3));
> +  testCases.push(StartTimeTest('3;; ', 3));
> +  testCases.push(StartTimeTest(';3', 3));
> +  testCases.push(StartTimeTest(' ;3', 3));

Looks like this version is now making us accept empty begin/end list entries now -- is that a correct interpretation of this change?

(Is that behavior interoperable/correct?  Might merit a code-comment here to document the expected new behavior.  Also: if this is a result of the SetBeginOrEndSpec changes, maybe it'd be worth splitting those code-changes and this test-change into a dedicated patch, since those are non-accessKey-specific and have a non-accessKey-related functionality difference?  And then the accessKey change can be 100% focused on accessKey)

Comment 9

[Brian Birtles (:birtles)]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> Comment on attachment 8936351 [details]
> Bug 1423098 - Drop support for SMIL accessKey;
> 
> https://reviewboard.mozilla.org/r/207068/#review213050
> 
> Can you explain the changes in this update? It's not clear to me why this
> affects the behavior. It looks like the code changes here are just adjusting
> how we report failure from SetBeginOrEndSpec, but it looks like we still end
> up reporting "some error code" in exactly the same conditions that we did
> before, so it's not clear to me why this produces a behavior change.  Do we
> react to different error codes differently, up the stack somewhere?

The key difference is that we drop the call to ClearSpecs(). Previously we'd call that and end up with no begin/end time (i.e. unresolved) but after this patch we will keep the specs we succeeded in parsing.

> ::: dom/smil/nsSMILTimedElement.cpp:1327
> (Diff revision 2)
> > -  nsresult rv = NS_OK;
> > -  while (tokenizer.hasMoreTokens() && NS_SUCCEEDED(rv)) {
> > +  bool hadFailure;
> > +  while (tokenizer.hasMoreTokens()) {
> >      nsAutoPtr<nsSMILTimeValueSpec>
> >        spec(new nsSMILTimeValueSpec(*this, aIsBegin));
> > -    rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
> > +    nsresult rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
> >      if (NS_SUCCEEDED(rv)) {
> >        timeSpecsList.AppendElement(spec.forget());
> > +    } else {
> > +      hadFailure = true;
> >      }
> >    }
> >  
> > -  if (NS_FAILED(rv)) {
> > -    ClearSpecs(timeSpecsList, instances, aRemove);
> > -  }
> > -
> > +  // The return value from this is only used to determine if we should print
> > +  // a console message or not, so we return failure if we had one or more
> > +  // failures.
> > +  return hadFailure ? NS_ERROR_FAILURE : NS_OK;
> 
> Two things
> 
> (1) I think you need to initialize hadFailure to false (optimistically) on
> the line where you declare it, right?

Yes, sorry. (I wrote this patch to try and keep me awake in the Stylo meeting, but that too failed.)

> (2) I didn't initially get the gist of the comment at the end. I think you
> should mention "so we don't bother distinguishing between different
> error-flavored nsresult values" or something along those lines.

Will do.

> ::: dom/smil/test/test_smilTiming.xhtml:135
> (Diff revision 2)
> > -  testCases.push(StartTimeTest('3;;', 'none'));
> > -  testCases.push(StartTimeTest('3;; ', 'none'));
> > -  testCases.push(StartTimeTest(';3', 'none'));
> > -  testCases.push(StartTimeTest(' ;3', 'none'));
> > +  testCases.push(StartTimeTest('3;;', 3));
> > +  testCases.push(StartTimeTest('3;; ', 3));
> > +  testCases.push(StartTimeTest(';3', 3));
> > +  testCases.push(StartTimeTest(' ;3', 3));
> 
> Looks like this version is now making us accept empty begin/end list entries
> now -- is that a correct interpretation of this change?

More or less. Basically, an empty begin/end list entry will not cause us to error out and drop the whole list of specs. It won't actually be added as a new spec. I should update the commit message.

> (Is that behavior interoperable/correct?  Might merit a code-comment here to
> document the expected new behavior.  Also: if this is a result of the
> SetBeginOrEndSpec changes, maybe it'd be worth splitting those code-changes
> and this test-change into a dedicated patch, since those are
> non-accessKey-specific and have a non-accessKey-related functionality
> difference?  And then the accessKey change can be 100% focused on accessKey)

Yeah, I'll do that.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Brian Birtles (:birtles) from comment #9)
> The key difference is that we drop the call to ClearSpecs().

Ah! I totally overlooked that removal. Thanks for clarifying!

Comment 11

[Brian Birtles (:birtles)]

Attachment: Bug 1423098 - Drop support for SMIL accessKey;

Review commit: https://reviewboard.mozilla.org/r/207546/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207546/

Comment 12

[Brian Birtles (:birtles)]

Comment on attachment 8936351 [details]
Bug 1423098 - Ignore invalid time value specifications rather than failing;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207068/diff/2-3/

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8936351 [details]
Bug 1423098 - Ignore invalid time value specifications rather than failing;

https://reviewboard.mozilla.org/r/207068/#review213434

::: commit-message-a44e0:3
(Diff revision 3)
> +This matches the behavior of Chrome at least, and will reduce compatibility
> +issues when we remove accessKey support in the next patch in this series since
> +specifications such as begin="3s; accessKey(s)" will continue to run the
> +animation.

I think this would be clearer/more-accurate with:
  s/since/so that/

(IIUC, this change is needed, in order for the animation to continue to run in that scenario.)

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8936845 [details]
Bug 1423098 - Drop support for SMIL accessKey;

https://reviewboard.mozilla.org/r/207546/#review213450

r=me

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8936351 [details]
Bug 1423098 - Ignore invalid time value specifications rather than failing;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207068/diff/3-4/

Comment 16

[Brian Birtles (:birtles)]

Comment on attachment 8936845 [details]
Bug 1423098 - Drop support for SMIL accessKey;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/207546/diff/1-2/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8fdaff3f0af
Ignore invalid time value specifications rather than failing; r=dholbert
https://hg.mozilla.org/integration/autoland/rev/6f8ed92515d6
Drop support for SMIL accessKey; r=dholbert

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/b8fdaff3f0af
https://hg.mozilla.org/mozilla-central/rev/6f8ed92515d6

Comment 19

[Daniel Holbert [:dholbert]]

I think we might want to consider uplift to beta here. What do you think, Brian?

(The patches seem pretty safe / minimally-invasive to me.  Minimal webcompat risk, given that we were the only ones shipping this feature.)

Comment 20

[Brian Birtles (:birtles)]

Comment on attachment 8936351 [details]
Bug 1423098 - Ignore invalid time value specifications rather than failing;

Approval Request Comment
[Feature/Bug causing the regression]: Bug 587910
[User impact if declined]: Potential for content to perform keylogging in contexts that are expected to be unscripted.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Both patches in this bug.
[Is the change risky?]: No
[Why is the change risky/not risky?]: Minimally invasive, simply drops a feature that other browsers never implemented, fails softly when no-longer-supported content is encountered.
[String changes made/needed]: None.

Comment 21

[Julien Cristau [:jcristau]]

It seems pretty late in beta to make a change affecting webcompat, I'd rather give this a chance to bake more than a couple of weeks in beta, i.e. ride the trains to 59.

Comment 22

[Chris Mills [:cmills]]

We don't really have any documentation about SMIL, and it certainly isn't a priority, so I've just added a note to the Fx59 rel notes for this:

https://developer.mozilla.org/en-US/Firefox/Releases/59#SVG_2

Let me know if that's OK. Thanks!