988768 - [tarako][dolphin][Sora][Message][MMS] Can't send messages with an attachment between 295 and 299KB

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Attachment: VID_0006.3gp

STR:

1. Create a message
2. Insert a video clip which size is between 295 and 299KB(you can use the attached one)
3. Try to send that MMS
4. The MMS cannot be sent out.

Comment 1

[Julien Wajsberg [:julienw]]

QAWanted for behavior in 1.1 and 1.3.

Please attach the logs with RIL debugging as well.

Thanks!

Comment 2

[Josh Schmitt [Joshs]]

(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #1)
> QAWanted for behavior in 1.1 and 1.3.
> 
> Please attach the logs with RIL debugging as well.
> 
> Thanks!

The issue does repro on the latest 1.3 and 1.1, the MMS is never sent.

1.3
Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140326004002
Gaia: 812838ad0fabf51fa14435af562ddac6d26fa936
Gecko: ba97efb0da4b
Version: 28.0
Firmware Version: V1.2-device.cfg

1.1
Environmental Variables:
Device: Buri 1.1 MOZ
BuildID: 20140318041202
Gaia: 44a2ddf63373f8e95c784faf4ed4d60081699c61
Gecko: 2c70ef07c5b3
Version: 18.0
Firmware Version: V1.2-device.cfg

Comment 3

[Julien Wajsberg [:julienw]]

Can we have the logs with RIL debugging please ? ;)

Bevis, do you think we should have a lower limit in Gaia? If the maximum size for a MMS is 300KB, what's the max attachment we can send?

Comment 4

[Jason Smith [:jsmith]]

FWIW - the video here doesn't showcase the problem here.

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #3)
> Can we have the logs with RIL debugging please ? ;)
> 
> Bevis, do you think we should have a lower limit in Gaia? If the maximum
> size for a MMS is 300KB, what's the max attachment we can send?

Actually, it cannot be a hard limitation to 300KB because we might have to 
take SMIL, text and http headers into consideration and we don't know how 
MMSC is configured in different operators to calculate the size.

It is not possible to have a general solution for this.
Hence, I'll suggest to leave this to vendor to configure for example leave 5KB 
to buffer for this case for different operators.

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Script to enable MMS/RIL debugging logs.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: tcpdump

Step to dump tcpdump:
  1. Install attached tcpdump command into test device:
     $adb remount; adb push tcpdump system/bin/;adb shell chmod 777 /system/bin/tcpdump
  2. Start capturing tcpdump:
     $adb shell tcpdump -i any -p -s 0 -w /data/capture.pcap
  3. backup tcpdump result:
     $adb pull /data/capture.pcap .

Comment 8

[Bevis Tseng[:bevis][:btseng](Exited)]

(In reply to Julien Wajsberg [:julienw] (away until March 24) from comment #3)
> Can we have the logs with RIL debugging please ? ;)
> 
Please apply the script in comment 6 to enable both ril/mms log and
apply the attached tcpdump in comment 7 to capture tcpdump.

Thanks!

Comment 9

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Attachment: MMSfailed.pcap

Comment 10

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Attachment: MMSfail.log

Comment 11

[Julien Wajsberg [:julienw]]

(In reply to Bevis Tseng [:bevistseng] ([e-mail]) from comment #5)
> (In reply to Julien Wajsberg [:julienw] (away until March 24) from comment
> #3)
> > Can we have the logs with RIL debugging please ? ;)
> > 
> > Bevis, do you think we should have a lower limit in Gaia? If the maximum
> > size for a MMS is 300KB, what's the max attachment we can send?
> 
> Actually, it cannot be a hard limitation to 300KB because we might have to 
> take SMIL, text and http headers into consideration and we don't know how 
> MMSC is configured in different operators to calculate the size.
> 
> It is not possible to have a general solution for this.
> Hence, I'll suggest to leave this to vendor to configure for example leave
> 5KB 
> to buffer for this case for different operators.

I agree, but maybe we can have a sensible default in Gaia, for example 295KB instead of 300KB that we have now. I don't think this would change much for the user.

Comment 12

[Brogan Zumwalt [Inactive]]

Comment 9 and Comment 10 appear to fulfil QAWanted request. Removing keyword.

Comment 14

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

> I agree, but maybe we can have a sensible default in Gaia, for example 295KB
> instead of 300KB that we have now. I don't think this would change much for
> the user.

Dear Julien -

+1. So is it possible to have this modification for 1.3? This issue is now a cert blocker.

Thanks

Vance

Comment 15

[Julien Wajsberg [:julienw]]

How can it be a cert blocker if it's behaving like this since v1.1 (see comment 2)? Moreover, the operator can change it in the apn.json file.

I mean, the patch should be trivial, we can do it, what I'm not understanding is why it's suddenly a cert blocker.

Comment 16

[Jason Smith [:jsmith]]

It isn't a blocker. We decided in triage that since the user is aware of an error when trying to attach media content > 295 KB that this issue was deemed minor.

Comment 17

[Julien Wajsberg [:julienw]]

Attachment: github PR

Comment 18

[Julien Wajsberg [:julienw]]

Vance, can you please ask the partner to test this PR with some workloads? Especially try workloads around the 295KB limit, to know whether we need to lower it to 294KB for example.

Comment 19

[Julien Wajsberg [:julienw]]

(In reply to Jason Smith [:jsmith] from comment #16)
> It isn't a blocker. We decided in triage that since the user is aware of an
> error when trying to attach media content > 295 KB that this issue was
> deemed minor.

Actually, currently, the user is aware of an error only once he tries to send the message, and it's rejected by the network. Asking 1.3? again for proper triage. (I still think it should not block, see comment 15, but it's not my call).

Comment 20

[Jason Smith [:jsmith]]

That's not still not a blocker for exactly the same reasons I said in comment 16 since there is user feedback here.

Comment 21

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Julien Wajsberg [:julienw] from comment #18)
> Vance, can you please ask the partner to test this PR with some workloads?
> Especially try workloads around the 295KB limit, to know whether we need to
> lower it to 294KB for example.

Hi Julien -

Thanks for providing the PR in such a short time. I already asked the partner to test the PR with different workloads. Will update the result soon

Also although we have this problem since 1.1, the reason it is not a blocker in 1.1 is because at that time this issue is not spotted by carriers, now they find it during the IOT test, and according to carriers, the reason they block on this one is because:

To the end users, they only know that the outgoing MMS size limitation is 300K, so naturally they assume that the MMS will send out successfully if they just insert some media files smaller than 300K(for example, 298K). However, the actual size of MMS is the media size + SMIL tag overhead size. So even if the user just insert a 298K size media object, the total size of MMS might exceed 300K. This will confuse end users, they don't know why MMS cannot send out successfully since they only insert a 29XK media object

Hope this explains the rational behind blocking this issue

Comment 22

[Jason Smith [:jsmith]]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #21)
> (In reply to Julien Wajsberg [:julienw] from comment #18)
> > Vance, can you please ask the partner to test this PR with some workloads?
> > Especially try workloads around the 295KB limit, to know whether we need to
> > lower it to 294KB for example.
> 
> Hi Julien -
> 
> Thanks for providing the PR in such a short time. I already asked the
> partner to test the PR with different workloads. Will update the result soon
> 
> Also although we have this problem since 1.1, the reason it is not a blocker
> in 1.1 is because at that time this issue is not spotted by carriers, now
> they find it during the IOT test, and according to carriers, the reason they
> block on this one is because:

Usually in the case where something isn't a regression needs a really strong compelling reason why it's needed now with data to back it up. Data such as support complaints.

> 
> To the end users, they only know that the outgoing MMS size limitation is
> 300K, so naturally they assume that the MMS will send out successfully if
> they just insert some media files smaller than 300K(for example, 298K).
> However, the actual size of MMS is the media size + SMIL tag overhead size.
> So even if the user just insert a 298K size media object, the total size of
> MMS might exceed 300K. This will confuse end users, they don't know why MMS
> cannot send out successfully since they only insert a 29XK media object

I think this analysis is missing a key piece of information - what exactly is happening when you try to send a 298 KB image in a MMS message right now? Do we spin infinitely with no end in sight? Do we get an error?

Comment 23

[Julien Wajsberg [:julienw]]

Nothing to answer, waiting for the partner's feedback before requesting review.

Comment 24

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Jason Smith [:jsmith] from comment #22)
> (In reply to Vance Chen [:vchen][[e-mail]] from comment #21)
> > (In reply to Julien Wajsberg [:julienw] from comment #18)
> > > Vance, can you please ask the partner to test this PR with some workloads?
> > > Especially try workloads around the 295KB limit, to know whether we need to
> > > lower it to 294KB for example.
> > 
> > Hi Julien -
> > 
> > Thanks for providing the PR in such a short time. I already asked the
> > partner to test the PR with different workloads. Will update the result soon
> > 
> > Also although we have this problem since 1.1, the reason it is not a blocker
> > in 1.1 is because at that time this issue is not spotted by carriers, now
> > they find it during the IOT test, and according to carriers, the reason they
> > block on this one is because:
> 
> Usually in the case where something isn't a regression needs a really strong
> compelling reason why it's needed now with data to back it up. Data such as
> support complaints.
> 
> > 
> > To the end users, they only know that the outgoing MMS size limitation is
> > 300K, so naturally they assume that the MMS will send out successfully if
> > they just insert some media files smaller than 300K(for example, 298K).
> > However, the actual size of MMS is the media size + SMIL tag overhead size.
> > So even if the user just insert a 298K size media object, the total size of
> > MMS might exceed 300K. This will confuse end users, they don't know why MMS
> > cannot send out successfully since they only insert a 29XK media object
> 
> I think this analysis is missing a key piece of information - what exactly
> is happening when you try to send a 298 KB image in a MMS message right now?
> Do we spin infinitely with no end in sight? Do we get an error?

No, we won't get an error prompt to indicate what went wrong, as mentioned in Comment#2, the MMS just never sent out

Comment 25

[Jason Smith [:jsmith]]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #24)
> No, we won't get an error prompt to indicate what went wrong, as mentioned
> in Comment#2, the MMS just never sent out

So what happens here then? Does the MMS sending process infinitely spin forever? Does the MMS look like it's sent, but not actually be sent?

Comment 26

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Jason Smith [:jsmith] from comment #25)
> (In reply to Vance Chen [:vchen][[e-mail]] from comment #24)
> > No, we won't get an error prompt to indicate what went wrong, as mentioned
> > in Comment#2, the MMS just never sent out
> 
> So what happens here then? Does the MMS sending process infinitely spin
> forever? Does the MMS look like it's sent, but not actually be sent?

The MMS will keep sending for a a while due to the retry mechanism, but eventually it will fail, you will see a fail icon in that MMS bubble.

Comment 27

[Jason Smith [:jsmith]]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #26)
> (In reply to Jason Smith [:jsmith] from comment #25)
> > (In reply to Vance Chen [:vchen][[e-mail]] from comment #24)
> > > No, we won't get an error prompt to indicate what went wrong, as mentioned
> > > in Comment#2, the MMS just never sent out
> > 
> > So what happens here then? Does the MMS sending process infinitely spin
> > forever? Does the MMS look like it's sent, but not actually be sent?
> 
> The MMS will keep sending for a a while due to the retry mechanism, but
> eventually it will fail, you will see a fail icon in that MMS bubble.

Fair enough - that counts as poor user feedback then.

So moving to blocking for being a cert blocker then.

Comment 28

[Steve Chung [:steveck]]

Comment on attachment 8401443 [details] [review]
github PR

If we really want to consider an estimated size for the mms header/smil/..., please also substract 5KB for Settings.mmsSizeLimitation. Value comes from settings operatorSizeLimitation would be missed the substraction in the patch.

Comment 29

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi all -

Thanks for your kindly help on this issue, our partner decides to take this one and fix it. Again much appreciate your help!!

remove 1.3 nominate as well

Comment 30

[Julien Wajsberg [:julienw]]

(In reply to Vance Chen [:vchen][[e-mail]] from comment #29)
> Hi all -
> 
> Thanks for your kindly help on this issue, our partner decides to take this
> one and fix it. Again much appreciate your help!!
> 
> remove 1.3 nominate as well

Sorry Vance, I don't understand the meaning of this.

What does it mean "the partner decides to take this one and fix it" ?

Also, I'm still waiting for a feedback from the partner about the proposed pull request.

Thanks!

Comment 31

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #28)
> Comment on attachment 8401443 [details] [review]
> github PR
> 
> If we really want to consider an estimated size for the mms header/smil/...,
> please also substract 5KB for Settings.mmsSizeLimitation. Value comes from
> settings operatorSizeLimitation would be missed the substraction in the
> patch.

My idea was that we did not a substraction ourselves, and use the operatorSizeLimitation as is if it was specified. I don't like the idea of doing automatic substraction but I may be wrong!

I don't know how it is specified: is it something that comes from apn.json, or is it something the operator will set himself? In the first case, we could go and change all values in our apn.json, while in the second case we can educate the operators.

Or rightly we can decrease the value automatically.

Steve what do you think?

Comment 32

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Julien Wajsberg [:julienw] from comment #30)
> (In reply to Vance Chen [:vchen][[e-mail]] from comment #29)
> > Hi all -
> > 
> > Thanks for your kindly help on this issue, our partner decides to take this
> > one and fix it. Again much appreciate your help!!
> > 
> > remove 1.3 nominate as well
> 
> Sorry Vance, I don't understand the meaning of this.
> 
> What does it mean "the partner decides to take this one and fix it" ?
> 
> Also, I'm still waiting for a feedback from the partner about the proposed
> pull request.
> 
> Thanks!

Hi Julien, my bad, i mean TCL decide to fix this issue by themselves. And your patch works great. However, as Steve mentioned in Comment#28, probably TCL still needs to substract some KB for Settings.mmsSizeLimitation. TCL knows that Mozilla are about to close 1.3 and we are focusing on fixing more important IOT blockers, that's why they decide to do the modification their own. I really hope that decision won't offend anyone anyone who contribute thoughts/patches to this bug. 

Thanks and please let me know if you have any comments

Vance

Comment 33

[Julien Wajsberg [:julienw]]

I'm always worried when TCL decides to do a modification by themselves. No offense intended here, but even for us at Mozilla there is a review by the peers.

Comment 34

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Julien Wajsberg [:julienw] from comment #33)
> I'm always worried when TCL decides to do a modification by themselves. No
> offense intended here, but even for us at Mozilla there is a review by the
> peers.

I share your concern too :), i will encourage TCL to share the patch with us so we can help to review as well

Comment 35

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #31)
> (In reply to Steve Chung [:steveck] from comment #28)
> > Comment on attachment 8401443 [details] [review]
> > github PR

> My idea was that we did not a substraction ourselves, and use the
> operatorSizeLimitation as is if it was specified. I don't like the idea of
> doing automatic substraction but I may be wrong!
> 
> I don't know how it is specified: is it something that comes from apn.json,
> or is it something the operator will set himself? In the first case, we
> could go and change all values in our apn.json, while in the second case we
> can educate the operators.
> 
> Or rightly we can decrease the value automatically.
> 
> Steve what do you think?

The operatorSizeLimitation should comes from apn.json IIRC, and we generate it automatically depending on some other resource(like opertator_variant.xml and other 3rd party apn resource). I'm not sure it's a wise decision to adjust the limitation when generating apn.json because it makes our apn information different then others resource. Maybe Jose could provide some comments for that?

Comment 36

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Steve Chung [:steveck] from comment #35)

> The operatorSizeLimitation should comes from apn.json IIRC, and we generate
> it automatically depending on some other resource(like opertator_variant.xml
> and other 3rd party apn resource). I'm not sure it's a wise decision to
> adjust the limitation when generating apn.json because it makes our apn
> information different then others resource. Maybe Jose could provide some
> comments for that?

The limit value is set by the carrier as a certification requirement. The source values are included in the `operator_variant.xml` are set by the carrier and used by Gaia through the `apn.json` database we build. If you guys need mode information about why this setting is needed (requested by the carrier) Beatriz (:brg) could provide it.

Comment 37

[Julien Wajsberg [:julienw]]

ok then, I'll substract manually !

Comment 38

[Steve Chung [:steveck]]

Comment on attachment 8401443 [details] [review]
github PR

Just one nit: https://github.com/mozilla-b2g/gaia/pull/17954#discussion_r11625388 and I think overall is good. So r=me and thanks!

Comment 39

[Julien Wajsberg [:julienw]]

Steve, I don't want to land without your final approval over the nit :)

Comment 40

[Steve Chung [:steveck]]

Comments in https://github.com/mozilla-b2g/gaia/pull/17954#r11667357
The reason why I think inNaN could fit this case because you want to check the parameter is exist and valid number.

Comment 41

[Julien Wajsberg [:julienw]]

(changed according to the discussion and waiting for a green travis)

Comment 42

[Julien Wajsberg [:julienw]]

master: cd04795ba688400ebb74c6d414fc49e25ad4279f

Comment 43

[Wei Gao (Spreadtrum)]

(In reply to Julien Wajsberg [:julienw] (away June 21 to 30) from comment #42)
> master: cd04795ba688400ebb74c6d414fc49e25ad4279f

Hi Julien

I think v1.3t and v1.4 also need this patch, 
Can this patch be merged into v1.3 and v1.4?

Thanks a great.

Comment 44

[Julien Wajsberg [:julienw]]

per partner's request

Comment 45

[Julien Wajsberg [:julienw]]

Comment on attachment 8401443 [details] [review]
github PR

per partner's request

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): -
[User impact] if declined: in some conditions, the recorded video is too big to be sent by MMS, even that the user followed the expected way.
[Testing completed]: yes
[Risk to taking this patch] (and alternatives if risky): quite low. Alternative is to lower the MMS size in apn.json.
[String changes made]: none

Comment 46

[Julien Wajsberg [:julienw]]

Note that the blocker status has been rejected twice already: see comment 16 and comment 20

Comment 47

[James Zhang (Spreadtrum)]

Wei, land WIP patch on our side.

Comment 48

[Lawrence Mandel [:lmandel] (use needinfo)]

The only bugs currently being accepted on the 1.4 branch are 1.4+ and 1.3+ with an approval request. I'm going to wait for triage to review the current 1.3? before reviewing the 1.4 approval request.

Comment 49

[Joe Cheng [:jcheng] (please needinfo)]

since the risk is low, taking this for tarako. i will leave the 1.4 decision to dolphin team

Comment 50

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8401443 [details] [review]
github PR

This bug is now 1.3+ and has a partner request on it for 1.4. I agree that it also looks safe to take. Approved for 1.4.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

v1.4: https://github.com/mozilla-b2g/gaia/commit/f250750c9b8381560fa33b2383737736e4dbdff5

Needs rebasing for v1.3t uplift, mostly in apps/sms/test/unit/settings_test.js.

Comment 52

[Julien Wajsberg [:julienw]]

Attachment: v1.3t github PR

Resolved conflicts
Will wait for Travis before merging

Comment 53

[Julien Wajsberg [:julienw]]

v1.3t: eca0212a0066d87cac9773cf6a67b10edc63c620