823816 - B2G MMS: WSP Text decode check the wrong range of ASCII code.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

WspPduHelper parse the decodeConstrainedMedia like 0x9d813100 will cause wrong result=>WspPduHelper: contentType = {"media":"1","params":null}

The root cause is the wrong range of ASCII in Text.decode.
After change the range check statement from "if ((code >= CTLS) && (code != DEL)) {" to "if ((code >= CTLS) && (code <= DEL)) {", I can fix the bug.

It shows the correct result like "WspPduHelper: contentType = {"media":"image/gif","params":null}".

Comment 1

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Fix the wrong ASCII range

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 694690 [details] [diff] [review]
Fix the wrong ASCII range

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

It's not. RFC-2616 has following definitions clearly:

  OCTET = <any 8-bit sequence of data>
  TEXT  = <any OCTET except CTLs, but including LWS>

So TEXT can definitely be non-ASCII, 8-bit characters. Instead, BNF of Extension-media should be:

  Extension-media = Text-string

to solve this case perfectly. However, I can't find any update to WAP-230-WSP-20010705-a now. Another solution would be try decoding ShortInteger before NullTerminatedTexts as Android does. Again, that's not specified in WSP spec.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

I don't think this issue should ever block bug 801632.

Comment 4

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Please see below codes for how Android dealing this issue for reference.
In function "parseContentType" in frameworks/opt/mms/src/java/com/google/android/mms/pdu/PduParser.java shows 
            if ((first >= TEXT_MIN) && (first <= TEXT_MAX)) {
                contentType = parseWapString(pduDataStream, TYPE_TEXT_STRING);
            } else if (first > TEXT_MAX) {
                int index = parseShortInteger(pduDataStream);
            ....
            }
Android check the first char to decide taking as WapString or ShortInteger. Maybe we can consider this method.

Comment 5

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Replace NullTerminatedTexts witn TextString

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 694732 [details] [diff] [review]
Replace NullTerminatedTexts witn TextString

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

Thank you! But since this is a non-standard solution, we should also update comments for |this.ConstrainedEncoding|.

::: dom/mms/src/ril/WspPduHelper.jsm
@@ +881,5 @@
>     *
>     * @return Decode integer value or string.
>     */
>    decode: function decode(data) {
> +    return decodeAlternatives(data, null, TextString, ShortInteger);

Could you also updates encode()? TypeValue.encode() should call ConstrainedEncoding.encode(), and ConstrainedEncoding.encode() should either call ShortInteger.encode() or TextString.encode().

Comment 7

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: Replace NullTerminatedTexts witn TextString

Comment 8

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 695408 [details] [diff] [review]
Replace NullTerminatedTexts witn TextString

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

\(^^)/

Comment 9

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Try server...
https://tbpl.mozilla.org/?tree=Try&rev=2628f2117a02

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b86cca9cd8d2

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b86cca9cd8d2

Comment 12

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

For mms merge to b2g18.

Comment 13

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Attachment: b2g18-004-bug-823816-fix

Comment 14

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/fd1db313bad5

Comment 15

[Ryan VanderMeulen [:RyanVM]]

The entire set of clian's pushes was backed out for multiple reasons.
https://tbpl.mozilla.org/?tree=Mozilla-B2g18&rev=a0b06192f882

1.) The tree rules are clear that you are not to land on top of bustage. At the time you pushed, both B2G Mn and B2G xpcshell had bustage from prior commits that hadn't been backed out yet.
2.) The tree rules are also clear that you are to watch your pushes for any bustage and handle them accordingly. mozilla-inbound is the ONLY tree where this rule does not apply.
3.) Even after the earlier bustage was backed out, something in one of your many pushes was causing further B2G Mn failures as shown in the log below.
https://tbpl.mozilla.org/php/getParsedLog.php?id=20424173&tree=Mozilla-B2g18
4.) This isn't cause for backout by itself, but it is also strongly preferred to not push each commit individually as our build and testing resources are limited and doing so stretches them even thinner. Please limit your number of pushes as much as possible unless you have good reason for keeping them separate.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/99680a32f297