Closed Bug 950105 Opened 7 years ago Closed 7 years ago

[Wap push] SI messages which contain '&' symbol in the URL are not received

Categories

(Firefox OS Graveyard :: RIL, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:koi+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 verified, b2g-v1.3 fixed)

VERIFIED FIXED
1.3 C1/1.4 S1(20dec)
blocking-b2g koi+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- verified
b2g-v1.3 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

()

Details

Attachments

(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #944422 +++

While investigating bug 944422 I found an issue in how WBXML code is parsed and decoded into a regular XML file. The WAP-192-WBXML-20010725-a standard [1] mandates that during encoding "character entities in the textual markup (e.g., &) which
can be represented in the target character encoding MUST be converted to string form". In practice this means that an '&' sequence in the original markup will be turned into the '&' character in the WBXML message.

Once the WBXML message is parsed and turned again into XML this process must be reversed for text fields and the following characters need to be turned into their character entity form:

& -> &
< -> &lt;
> -> &gt;

If we fail to do this the resulting XML document will be invalid as &, < and > are not permitted to appear unescaped in a text field.

To fix this problem I believe we should detect and replace these characters with character entities in both WbxmlInlineString.decode() [2] and WbxmlStringTable.decode() [3]. The relevant unit-tests in test_si_pdu_helper.js and test_sl_pdu_helper.js should also be augmented to cover these cases.

This bug is currently causing WAP Push messages containing URL with search parameters (which thus include the '&' character) to be dropped by the WAP Push app as the DOMParser refuses the XML file delivered by gecko. I'm marking this bug as koi? since the original was a koi+.

I'd tackle this bug myself but I spent most of today trying to run the xpcshell test within the emulator with no success so I'm not sure how long it could take me. Maybe someone more familiar with this code can take over.

[1] http://technical.openmobilealliance.org/tech/affiliates/wap/wap-192-wbxml-20010725-a.pdf
[2] http://hg.mozilla.org/mozilla-central/file/8b5875dc7e31/dom/wappush/src/gonk/WbxmlPduHelper.jsm#l137
[3] http://hg.mozilla.org/mozilla-central/file/8b5875dc7e31/dom/wappush/src/gonk/WbxmlPduHelper.jsm#l119
Triage Notes:

Sounds like a blocker. Search parameters are a common use case for a URL, so this is could definitely happen.
This is a WIP patch that implements escaping reserved XML characters when parsing inline string and string table references. I'd like to add some tests to verify this works correctly but I've been unable to run the WAP Push xpcshell tests on my device or on the emulator.

I've followed the instructions on MDN https://developer.mozilla.org/en-US/Firefox_OS/Platform/Automated_testing/XPCShell but couldn't figure out what to pass to the --testing-modules-dir and --manifest parameters.

Chuck can you have a look at my patch and see if it looks reasonable? If you can also help me out with xpcshell tests I should be able to finish this one pretty soon.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Attachment #8347966 - Flags: feedback?(chulee)
Comment on attachment 8347966 [details] [diff] [review]
[PATCH WIP] Escape reserved characters when converting a WBXML document to XML

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

Look good to me, and thanks for fixing this.

xpcshell for WAP Push can't run in emulator is caused by bug 948376, and is fixed now.
you can add test case to dom/wappush/tests/test_cp_pdu_helper.js, and use run test by following command after build emulator.

./test.sh xpcshell --test-path B2G_EMULATOR_PATH/objdir-gecko/dist/test-package-stage/xpcshell/tests/dom/wappush/tests/test_cp_pdu_helper.js
Attachment #8347966 - Flags: feedback?(chulee) → feedback+
(In reply to Chuck Lee [:chucklee] from comment #3)
> xpcshell for WAP Push can't run in emulator is caused by bug 948376, and is
> fixed now.
> you can add test case to dom/wappush/tests/test_cp_pdu_helper.js, and use
> run test by following command after build emulator.
> 
> ./test.sh xpcshell --test-path
> B2G_EMULATOR_PATH/objdir-gecko/dist/test-package-stage/xpcshell/tests/dom/
> wappush/tests/test_cp_pdu_helper.js

Ah, great! Thanks!
Here's an update patch complete with an additional xpcshell test to exercise this functionality.

Enpei can you check if this patch fixes the issue you tested in bug 944422? The patch I'm attaching applies cleanly to gecko in the 1.2 branch. The behavior should now be identical to Android: when the 'href' field points to an URL with the '&' symbol it will be properly translated and the resulting link in the WAP Push app should be shown and should be identical to what you see in Android.
Attachment #8347966 - Attachment is obsolete: true
Attachment #8349469 - Flags: review?(chulee)
Flags: needinfo?(echu)
Comment on attachment 8349469 [details] [diff] [review]
[PATCH] Escape reserved characters when converting a WBXML document to XML

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

Look good to me, Thanks!
But I am not a peer, add vicamo as reviewer.
Attachment #8349469 - Flags: review?(vyang)
Attachment #8349469 - Flags: review?(chulee)
Attachment #8349469 - Flags: review+
Hi Gabriele,

Verified on Buri with attached patch.
Gaia      c82c957e9d4078cd4043de6b7d21a2667a73adf7
Gecko     65dfa0844f0abbe4cfc90708f8620df4811afe6a
BuildID   20131219140435
Version   26.0
ro.build.version.incremental=291
ro.build.date=Wed Dec  4 09:48:04 CST 2013

Nicely work and it will display exactly what user send no matter "&" or "&amp". Also verified other possible special characters(! * ' ( ) ; : @ & = + $ , / ? % # [ ]- _ . ~).
Flags: needinfo?(echu)
Comment on attachment 8349469 [details] [diff] [review]
[PATCH] Escape reserved characters when converting a WBXML document to XML

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

r=chulee,me. Thank you :)
Attachment #8349469 - Flags: review?(vyang) → review+
Thanks Chuck & Vicamo for the review and big thanks to Enpei for her thorough testing!

The green try-run is here: https://tbpl.mozilla.org/?tree=Try&rev=0272bad783dc so setting checkin-needed; this needs to land on master and on mozilla-b2g26_v1_2, the patch should apply cleanly everywhere. I've set the flags accordingly, once landed the issue should be fixed across all affected versions.
Keywords: checkin-needed
koi+. please uplift.
blocking-b2g: koi? → koi+
https://hg.mozilla.org/mozilla-central/rev/778e7a7b267e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(In reply to Wes Kocher (:KWierso) from comment #12)
> https://hg.mozilla.org/mozilla-central/rev/778e7a7b267e

Hi Wes, could you also help to uplift to v1.2 due to this is a koi+ bug. Thanks.
Flags: needinfo?(kwierso)
Verified on Buri v1.2 and passed.

Gaia      01e9da49be2cc4bc134eeefc434740d572ec2246
Gecko     252332f9d779541d82b87b9e17b57a4d5a978443
BuildID   20131223095701
Version   28.0a2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.