1237546 - SapiService::Speak should encode to entities

Status: RESOLVED FIXED

(Core :: Web Speech, defect)

Description

[Makoto Kato [:m_kato]]

To support pitch controll, we use XML for Speak parameter on SAPI backend.  We should encode some character to entity

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Convert character to entity for SapiService::Speak

Comment 2

[Makoto Kato [:m_kato]]

Comment on attachment 8705049 [details] [diff] [review]
Convert character to entity for SapiService::Speak

When using speechSynthesis.speak(new SpeechSynthesisUtterance("&") on SAPI backend, browser speaks as '&' only.  It should be "&', 'a', 'm', 'p' and ';'.

Our SAPI implementation use XML to support pitch value.  So some characters should be converted to entity.

Comment 3

[Eitan Isaacson [:eeejay]]

Two questions before I review:
1. Don't we have an actual library for escaping XML? Seems like a very common and security sensitive thing to do.
2. Do Mac/Linux need followup patches as well? I think we may need to escape or sanitize SSML.

Comment 4

[Makoto Kato [:m_kato]]

(In reply to Eitan Isaacson [:eeejay] from comment #3)
> Two questions before I review:
> 1. Don't we have an actual library for escaping XML? Seems like a very
> common and security sensitive thing to do.

ContentUtils has internal method into StringBuilder, but it doesn't export to others and requires nsINode and nsTextFragment.  If we should add this to ContentUtils, I will add it.

> 2. Do Mac/Linux need followup patches as well? I think we may need to escape
> or sanitize SSML.

OSX API doesn't recognize SSML.

speechd can use SSML mode, but to use SSML, we need call spd_set_data_mode(handle, SPD_DATA_SSML).

Comment 5

[Eitan Isaacson [:eeejay]]

(In reply to Makoto Kato [:m_kato] from comment #4)
> (In reply to Eitan Isaacson [:eeejay] from comment #3)
> > Two questions before I review:
> > 1. Don't we have an actual library for escaping XML? Seems like a very
> > common and security sensitive thing to do.
> 
> ContentUtils has internal method into StringBuilder, but it doesn't export
> to others and requires nsINode and nsTextFragment.  If we should add this to
> ContentUtils, I will add it.
> 

I'm not sure. Let's see what smaug thinks.

Comment 6

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

Whatever StringBuilder does is very innerHTML specific and super performance hot code.
But, perhaps you could factor some of EncodeTextFragment to a macro which is then called twice in that method and used also for a method needed in this bug.
Or even better would be to not use macro, but some template method which should get inlined.

something like
void templace<T>EncodeText(const T aChar, nsAString& aOut) 
{
  switch (aChar) {
    case '<':
     aOut.AppendLiteral("&lt;");
     break;
    case '>':
      aOut.AppendLiteral("&gt;");
      break;
    case '&':
      aOut.AppendLiteral("&amp;");
      break;
    case 0x00A0:
      aOut.AppendLiteral("&nbsp;");
      break;
     default:
       aOut.Append(aChar);
       break;
  }
}

Comment 7

[Eitan Isaacson [:eeejay]]

Comment on attachment 8705049 [details] [diff] [review]
Convert character to entity for SapiService::Speak

Review of attachment 8705049 [details] [diff] [review]:
-----------------------------------------------------------------

This is good for now. We may need to figure out a way to share this across services or escape strings before they reach the service.

Comment 8

[Makoto Kato [:m_kato]]

mach try -b do -p win32,win64 -u all -t none

Comment 9

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fffc91006aebf41a8ab747f936ffdcfaa1e239a

Comment 10

[Eitan Isaacson [:eeejay]]

Do you plan to land this?

This might be last minute, but what do you think of using nsEscapeHTML2?

https://dxr.mozilla.org/mozilla-central/source/xpcom/io/nsEscape.h#56

Comment 11

[Makoto Kato [:m_kato]]

Attachment: Use nsEscapeHTML2 for speech text

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed42b270f7e

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ed42b270f7e