Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Description • 7 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 1 • 7 years ago

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

	Daniel Holbert [:dholbert]
	Comment 2 • 7 years ago

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

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 3 • 7 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 5 • 7 years ago
mozreview-review

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?)

	Kohei Yoshino [:kohei]
	Comment 6 • 7 years ago

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

	Daniel Holbert [:dholbert]
	Comment 8 • 7 years ago
mozreview-review

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)

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 9 • 7 years ago

(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.

	Daniel Holbert [:dholbert]
	Comment 10 • 7 years ago

(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!

	Daniel Holbert [:dholbert]
	Comment 13 • 6 years ago
mozreview-review

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.)

	Daniel Holbert [:dholbert]
	Comment 14 • 6 years ago
mozreview-review

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

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

r=me

	Pulsebot
	Comment 17 • 6 years ago

Pushed by bbirtles@mozilla.com:
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

	Andrei Ciure[:aciure]
	Comment 18 • 6 years ago
bugherder

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

	Daniel Holbert [:dholbert]
	Comment 19 • 6 years ago

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.)

	Brian Birtles (:birtles, Feb 26~ parental leave) Assignee
	Comment 20 • 6 years ago

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.

	Julien Cristau [:jcristau]
	Comment 21 • 6 years ago

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.

	Chris Mills [:cmills]
	Comment 22 • 6 years ago

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!