935873 - MMS retry should handle modified input stream

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Shao Hang Kao [:skao]]

Sometimes when xhr failed the size of input stream get smaller, should do something to re-construct the complete input stream.

Comment 1

[Shao Hang Kao [:skao]]

Attachment: bug935873.patch

Comment 2

[Shao Hang Kao [:skao]]

Gene,

I found that after compose, the part.content type will change to normal byte array. Could you have a look to check if this fix is feasible? Thanks!

Also I changed the retry time for the first retry to 3 seconds (where the default one is 300 seconds). And this can solve our resend issue as a work around.

Comment 3

[Gene Lian (I already quit Mozilla)]

Comment on attachment 828544 [details] [diff] [review]
bug935873.patch

Basically, we cannot change preference at will, which is for verdor's configurations. I'd prefer delegating this review to Vicamo.

Comment 4

[Shao Hang Kao [:skao]]

I didn't get it? I only change the first retry period, the following retries would follow the preference as well.

Comment 5

[Shao Hang Kao [:skao]]

summary short discussion with Vicamo:

1. should split the 3 seconds work around into another patch/bug
2. find out where part.content get modified and not to modify it
3. have a test case to make sure part.content won't get modified after compose

Comment 6

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

Comment on attachment 828544 [details] [diff] [review]
bug935873.patch

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

For TIME_FOR_FIRST_RETRY thing, that's not belong to this bug.  I think we can all agree that Gecko shall not produce surprises to Gaia by changing user preferences at will.

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +1206,5 @@
> +          // we have to re-compose it.
> +          if (this.istreamSize == null ||
> +              this.istreamSize != this.istream.available()) {
> +            this.istream = MMS.PduHelper.compose(null, this.msg);
> +            this.istreamSize = this.istream.available();

Shouldn't we have the same |this.istreamSize|?

::: dom/mobilemessage/src/gonk/WspPduHelper.jsm
@@ +2455,4 @@
>          }
>          part.content = this.encodeStringContent(part.content, charset);
>          UintVar.encode(data, part.content.length);
> +      } else if (part.content.length != null) {

I'd like to know which line overrides |part.content| and re-evaluates its necessity.  Keeping |msg| unchanged throughout the PDU composing process might help us in further problems.

Besides, there is a xpcshell test script "dom/mobilemessage/tests/test_wsp_pdu_helper.js" for this jsm.  Please also add a test case for this. :)

Comment 7

[Shao Hang Kao [:skao]]

Attachment: bug935873.patch

Hi Vicamo,

I'll upload another patch once the tests finished, Thanks!

Comment 8

[Shao Hang Kao [:skao]]

Attachment: bug935873.patch

Comment 9

[Shao Hang Kao [:skao]]

Attachment: bug935873.patch

Comment 10

[Shao Hang Kao [:skao]]

Hi Vicamo,

I accidentally removed the re-compose part from the patch, could you help to review it again?
Thanks!

Comment 11

[Gene Lian (I already quit Mozilla)]

Just a drive-by review. nit: s/the input stream.../The input stream.../

Comment should follow: // {upper-case-letter}...{a period}.

Comment 12

[Gene Lian (I already quit Mozilla)]

Btw, if you've already got review then don't need to ask for review again until you have extra changes.

Comment 13

[Shao Hang Kao [:skao]]

Yes, there are extra changes. I missed re-compose part in previous patches!

Comment 14

[Shao Hang Kao [:skao]]

Attachment: bug935873_test.patch

Comment 15

[Shao Hang Kao [:skao]]

Attachment: bug935873.patch

fixed the nit Gene mentioned.

Comment 16

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

Comment on attachment 830721 [details] [diff] [review]
bug935873.patch

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

r=me, and just a remind: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 17

[Shao Hang Kao [:skao]]

Attachment: bug935873_test.patch

Comment 18

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

Comment on attachment 831314 [details] [diff] [review]
bug935873_test.patch

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

::: dom/mobilemessage/tests/test_wsp_pdu_helper.js
@@ +1353,5 @@
> +  let afterCompose = JSON.stringify(parts);
> +
> +  if (beforeCompose !== afterCompose) {
> +    do_throw("expect value: '" + beforeCompose + "', got '" + afterCompose + "'");
> +  }

nit: do_check_eq(beforeCompose, afterCompose);

I also find this might come from "dom/mobilemessage/tests/header_helpers.js:function wsp_test_func".  Please help update that as well.

Comment 19

[Shao Hang Kao [:skao]]

Attachment: bug935873_test.patch

fixed nits, will upload final patches for check-in soon.

Comment 20

[Shao Hang Kao [:skao]]

Attachment: bug935873.patch (ready for checkin)

Comment 21

[Shao Hang Kao [:skao]]

Attachment: bug935873_test.patch (ready for checkin)

Comment 22

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

https://hg.mozilla.org/integration/b2g-inbound/rev/20f754fcefb5
https://hg.mozilla.org/integration/b2g-inbound/rev/1a9bf903a453

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/20f754fcefb5
https://hg.mozilla.org/mozilla-central/rev/1a9bf903a453