[Simulator] Add JSON validation code on TV Simulator Service.

RESOLVED FIXED in Firefox 44

Status

Firefox OS
Simulator
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mantaroh, Assigned: mantaroh)

Tracking

(Blocks: 1 bug)

unspecified
FxOS-S9 (16Oct)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

2 years ago
see bug 1180589 comment #45.

We should create validation code in order to prevent invalid setting file.
(Assignee)

Updated

2 years ago
Depends on: 1180124
(Assignee)

Comment 1

2 years ago
We will consider thing as follow:
 - JSON Format [1]. We can care json format using SyntaxError[2].
 - 'tuners' object              : Tolerate empty array.
 - 'tuners.id' string           : Tolerate empty string, but not allow duplicate other id.
 - 'tuners.supportedType' array : Tolerate empty array.
 - 'tuners.sources' array       : Tolerate empty array.
 - 'sources.type' string        : Not allow empty string.
 - 'sources.channels' array     : Tolerate empty array.
 - 'channels.networkId' string  : Tolerate empty string.
 - 'sources.transportStreamId'  : Ditto.
 - 'sources.serviceId'          : Ditto.
 - 'sources.type'               : Ditto.
 - 'sources.name'               : Ditto.
 - 'sources.number'             : Ditto.
 - 'sources.isEmergency'        : Ditto.
 - 'sources.isFree'             : Ditto.
 - 'sources.videoFilePath'      : MUST exist.
 - 'sources.programs'           : Tolerate empty array.
 - 'programs.eventId'           : Tolerate empty string
 - 'programs.title'             : Ditto.
 - 'programs.startTime'         : Ditto.
 - 'programs.duration'          : Ditto.
 - 'programs.description'       : Ditto.
 - 'programs.rating'            : Ditto.
 - 'programs.audioLanugages'    : Ditto.
 - 'programs.subtitleLanguages' : Ditto.

[1] http://json.org/
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SyntaxError
(Assignee)

Comment 2

2 years ago
Created attachment 8665808 [details] [diff] [review]
WIP:Add validation code in order to prevent invalid file.

The patch file is WIP.
So its file is adding the validation code.
(Assignee)

Comment 3

2 years ago
In dom/tv/TVType.cpp,
Some setter function is not allow the empty or null data[1] as follow:
[TVTunerData]
 - id
 - supportedSourceType
[TVChannelData]
 - networkId
 - transportStreamId
 - serviceid
 - type
 - number
 - name
[ProgramData]
 - eventId
 - title

Adding comment #1, we should validate above condition.

[1] https://dxr.mozilla.org/mozilla-central/rev/6256ec9113c115141aab089c45ee69438884b680/dom/tv/TVTypes.cpp#43
Assignee: nobody → mantaroh
(Assignee)

Comment 4

2 years ago
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d7be3b310a9
(Assignee)

Comment 5

2 years ago
Created attachment 8667737 [details] [diff] [review]
Add validation code in order to prevent invalid file.

Hi sean,
In bug 1180589 comment #45,
We should prevent invalid setting file.

So I added the validation code.

This validation will trigger after parse to JSON.
Attachment #8665808 - Attachment is obsolete: true
Attachment #8667737 - Flags: review?(selin)
Comment on attachment 8667737 [details] [diff] [review]
Add validation code in order to prevent invalid file.

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

::: dom/tv/TVSimulatorService.js
@@ +116,5 @@
>      }
>  
> +    // validation
> +    if (!this._validateSetting(settingObj)) {
> +      debug("Failed validate setting file.");

nit: Failed to validate settings.

@@ +439,5 @@
>  
>      return dsFile.path;
>    },
> +
> +  _validateSetting: function TVSimValidateSetting(aSettingObject) {

nit: _validateSetting => _validateSettings

@@ +445,5 @@
> +  },
> +
> +  _validateTuners: function TVSimValidateTuners(aTunersObject) {
> +    let tunerIds = new Array();
> +    for (let i=0; i<aTunersObject.length; i++) {

nit: add white spaces before and after = and <.

@@ +448,5 @@
> +    let tunerIds = new Array();
> +    for (let i=0; i<aTunersObject.length; i++) {
> +      if (!aTunersObject[i].id ||
> +          !aTunersObject[i].supportedType ||
> +          !aTunersObject[i].supportedType.length ||

Could we also ensure the types are all included in the allowed TV source types [1]?

[1] http://seanyhlin.github.io/TV-Manager-API/#idl-def-TVSourceType

@@ +463,5 @@
> +    return true;
> +  },
> +
> +  _validateSources: function TVSimValidateSources(aSourcesObject) {
> +    for (let i=0; i<aSourcesObject.length; i++) {

nit: add white spaces before and after = and <.

@@ +464,5 @@
> +  },
> +
> +  _validateSources: function TVSimValidateSources(aSourcesObject) {
> +    for (let i=0; i<aSourcesObject.length; i++) {
> +      if (!aSourcesObject[i].type) {

Could we also ensure the type is one of the allowed TV source types [1]?

[1] http://seanyhlin.github.io/TV-Manager-API/#idl-def-TVSourceType

@@ +477,5 @@
> +    return true;
> +  },
> +
> +  _validateChannels: function TVSimValidateChannels(aChannelsObject) {
> +    for (let i=0; i<aChannelsObject.length; i++) {

nit: add white spaces before and after = and <.

@@ +478,5 @@
> +  },
> +
> +  _validateChannels: function TVSimValidateChannels(aChannelsObject) {
> +    for (let i=0; i<aChannelsObject.length; i++) {
> +      if (!aChannelsObject[i].videoFilePath ||

Should we allow to have no video file path (to simulate no signal for that channel)?

@@ +482,5 @@
> +      if (!aChannelsObject[i].videoFilePath ||
> +          !aChannelsObject[i].networkId ||
> +          !aChannelsObject[i].transportStreamId ||
> +          !aChannelsObject[i].serviceId ||
> +          !aChannelsObject[i].type ||

Could we also ensure the type is one of the allowed TV channel types [1]?

[1] http://seanyhlin.github.io/TV-Manager-API/#idl-def-TVChannelType

@@ +483,5 @@
> +          !aChannelsObject[i].networkId ||
> +          !aChannelsObject[i].transportStreamId ||
> +          !aChannelsObject[i].serviceId ||
> +          !aChannelsObject[i].type ||
> +          !aChannelsObject[i].number ||

It'd better to also ensure no two channels share the same channel number.

@@ +493,5 @@
> +    return true;
> +  },
> +
> +  _validatePrograms: function TVSimValidatePrograms(aProgramsObject) {
> +    for( let i=0; i<aProgramsObject.length; i++ ) {

nit: add white spaces before and after = and <. And there's no need to add a white space after ( and before ).

@@ +494,5 @@
> +  },
> +
> +  _validatePrograms: function TVSimValidatePrograms(aProgramsObject) {
> +    for( let i=0; i<aProgramsObject.length; i++ ) {
> +      if (!aProgramsObject[i].eventId || !aProgramsObject[i].title) {

It'd better to also ensure no two programs of the channel share the same event ID. And please also check the start time and duration really exist.
Attachment #8667737 - Flags: review?(selin)
(Assignee)

Comment 7

2 years ago
Created attachment 8669518 [details] [diff] [review]
Add validation code in order to prevent invalid file.

(In reply to Sean Lin [:seanlin] from comment #6)
> Comment on attachment 8667737 [details] [diff] [review]
> Add validation code in order to prevent invalid file.
> 
> Review of attachment 8667737 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +478,5 @@
> > +  },
> > +
> > +  _validateChannels: function TVSimValidateChannels(aChannelsObject) {
> > +    for (let i=0; i<aChannelsObject.length; i++) {
> > +      if (!aChannelsObject[i].videoFilePath ||
> 
> Should we allow to have no video file path (to simulate no signal for that
> channel)?
Did you mean that if have no video file path, stream will null ?
If settings have no video file path, the error will occur in the TVSimulatorService.getSimulatorVideoBlobURL[1].
And tuner's stream will null.

I think it would better to allow the no video file path.
So I removed the validation of videoFilePath.

[1] https://dxr.mozilla.org/mozilla-central/rev/3d7532ce81ac571962abc3b99582fe7f5d685192/dom/tv/TVSimulatorService.js?offset=200#410


> @@ +494,5 @@
> > +  },
> > +
> > +  _validatePrograms: function TVSimValidatePrograms(aProgramsObject) {
> > +    for( let i=0; i<aProgramsObject.length; i++ ) {
> > +      if (!aProgramsObject[i].eventId || !aProgramsObject[i].title) {
> 
> It'd better to also ensure no two programs of the channel share the same
> event ID. And please also check the start time and duration really exist.
Oh, I misunderstood that TV Manager API can allow the duplicate channel number.
I added the validation of duplication.
Attachment #8667737 - Attachment is obsolete: true
Attachment #8669518 - Flags: review?(selin)
Comment on attachment 8669518 [details] [diff] [review]
Add validation code in order to prevent invalid file.

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

r=me when the corresponding updates are made for the following comments.

::: dom/tv/TVSimulatorService.js
@@ +22,5 @@
> +function containInvalidSourceType(element, index, array) {
> +  if (!TV_SOURCE_TYPES.includes(element)) {
> +    return true;
> +  }
> +  return false;

nit: how about this?
 
function isInvalidSourceType(aElement, aIndex, aArray) {
  return !TV_SOURCE_TYPES.includes(aElement);
}

@@ +454,5 @@
>  
>      return dsFile.path;
>    },
> +
> +  _validateSettings: function TVSimValidateSetting(aSettingsObject) {

nit: TVSimValidateSetting => TVSimValidateSettings

@@ +496,5 @@
> +
> +  _validateChannels: function TVSimValidateChannels(aChannelsObject) {
> +    let channelNumbers = new Array();
> +    for each (let channel in aChannelsObject) {
> +      if (!channel.videoFilePath ||

> > Should we allow to have no video file path (to simulate no signal for that
> > channel)?
> Did you mean that if have no video file path, stream will null ?
Yes.

> If settings have no video file path, the error will occur in the
> TVSimulatorService.getSimulatorVideoBlobURL[1].
> And tuner's stream will null.
> 
> I think it would better to allow the no video file path.
> So I removed the validation of videoFilePath.
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> 3d7532ce81ac571962abc3b99582fe7f5d685192/dom/tv/TVSimulatorService.
> js?offset=200#410
> 
So please remove this check. |videoFilePath| can be unset.

Besides, you may add |!wrapChannelData.videoFilePath| at [2] to more gracefully address this case and then return an empty string for |getSimulatorVideoBlobURL| call.

[2] https://dxr.mozilla.org/mozilla-central/rev/3d7532ce81ac571962abc3b99582fe7f5d685192/dom/tv/TVSimulatorService.js#406
Attachment #8669518 - Flags: review?(selin) → review+
(Assignee)

Comment 9

2 years ago
Created attachment 8670066 [details] [diff] [review]
Add validation code in order to prevent invalid file.

(In reply to Sean Lin [:seanlin] from comment #8)
> Comment on attachment 8669518 [details] [diff] [review]
> Add validation code in order to prevent invalid file.
> 
> Review of attachment 8669518 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me when the corresponding updates are made for the following comments.
Thank you for your confirmation.

> So please remove this check. |videoFilePath| can be unset.
> 
> Besides, you may add |!wrapChannelData.videoFilePath| at [2] to more
> gracefully address this case and then return an empty string for
> |getSimulatorVideoBlobURL| call.
> 
> [2]
> https://dxr.mozilla.org/mozilla-central/rev/
> 3d7532ce81ac571962abc3b99582fe7f5d685192/dom/tv/TVSimulatorService.js#406
I added the videoFilePath validation to |GetSimuatorVideoBlobURL|.
In addition to this, I added validation of empty string in TVTuner.cpp[1].
Because an error will occurr when we set empty string to video element's src attribute.[2]

[1] https://dxr.mozilla.org/mozilla-central/rev/d56c816b35c34133a9249663b2c19830a29b133a/dom/tv/TVTuner.cpp#288
[2] https://dxr.mozilla.org/mozilla-central/rev/d56c816b35c34133a9249663b2c19830a29b133a/dom/html/HTMLMediaElement.cpp#954
Attachment #8669518 - Attachment is obsolete: true
Attachment #8670066 - Flags: review?(selin)
Comment on attachment 8670066 [details] [diff] [review]
Add validation code in order to prevent invalid file.

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

::: dom/tv/TVSimulatorService.js
@@ +22,5 @@
> +function containInvalidSourceType(aElement, aIndex, aArray) {
> +  if (!TV_SOURCE_TYPES.includes(aElement)) {
> +    return true;
> +  }
> +  return false;

nit: |return !TV_SOURCE_TYPES.includes(aElement)| looks well enough.

::: dom/tv/TVTuner.cpp
@@ +284,5 @@
>                                                       ToTVSourceTypeStr(mCurrentSource->Type()),
>                                                       currentChannelNumber,
>                                                       domWin,
>                                                       currentVideoBlobUrl);
> +  if (NS_WARN_IF(NS_FAILED(rv)) || currentVideoBlobUrl.IsEmpty()) {

nit: NS_WARN_IF(NS_FAILED(rv) || currentVideoBlobUrl.IsEmpty())
Attachment #8670066 - Flags: review?(selin) → review+
(Assignee)

Comment 11

2 years ago
Created attachment 8670108 [details] [diff] [review]
Add validation code in order to prevent invalid file.

(In reply to Sean Lin [:seanlin] from comment #10)
> Comment on attachment 8670066 [details] [diff] [review]
> Add validation code in order to prevent invalid file.
> 
> Review of attachment 8670066 [details] [diff] [review]:
> -----------------------------------------------------------------
Sean,
Thank you.

I update comment #10.
Carrying r+ forward.
Attachment #8670066 - Attachment is obsolete: true
Attachment #8670108 - Flags: review+

Comment 12

2 years ago
Created attachment 8670109 [details] [review]
[gaia] mantaroh:master > mozilla-b2g:master
(Assignee)

Comment 13

2 years ago
Comment on attachment 8670109 [details] [review]
[gaia] mantaroh:master > mozilla-b2g:master

Sean,

I forgot the modify Gaia's settings file.
Current settings file couldn't pass the validation.
(Duplicated eventId and typo 'networkId')
Attachment #8670109 - Flags: review?(selin)
Comment on attachment 8670109 [details] [review]
[gaia] mantaroh:master > mozilla-b2g:master

Hi Ricky,

Since you've reviewed relevant Gaia patches in bug 1180589, could you help with it as well?
Attachment #8670109 - Flags: review?(selin) → review?(rchien)

Updated

2 years ago
Blocks: 1180124
No longer depends on: 1180124
Comment on attachment 8670109 [details] [review]
[gaia] mantaroh:master > mozilla-b2g:master

I don't know how this things work but it seems fine to me for build part changes.
If you want to make sure this things work properly on Gaia, you probably ask Yifan I think he may know about that.
Attachment #8670109 - Flags: review?(rchien) → review+
(Assignee)

Comment 16

2 years ago
Hi Sean, Ricky
Thank you for your confirmation.
Keywords: checkin-needed
https://github.com/mozilla-b2g/gaia/commit/97443c22201991662cdbe08c4fab1b780685f0b2

does the gecko change also need to land ?
Flags: needinfo?(mantaroh)
(Assignee)

Comment 18

2 years ago
Hi Tomcat,

(In reply to Carsten Book [:Tomcat] from comment #17)
> https://github.com/mozilla-b2g/gaia/commit/
> 97443c22201991662cdbe08c4fab1b780685f0b2
> 
> does the gecko change also need to land ?
Yes,
We need to land Gaia and gecko parts.
So if you can, could you land the attachment #8670108 [details] [diff] [review]?
Flags: needinfo?(mantaroh) → needinfo?(cbook)

Comment 19

2 years ago
https://hg.mozilla.org/integration/b2g-inbound/rev/254a1117b6ae
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/254a1117b6ae
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S9 (16Oct)
(Assignee)

Comment 21

2 years ago
Clear unnecessary needinfo.
Flags: needinfo?(cbook)
You need to log in before you can comment on or make changes to this bug.