852911 - B2G MMS: fail to expose correct nsIDOMMozMmsMessage.attachments

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Hi Blake,

If we don't do what I did in this patch, when the |attachments| passed into the MmsMessage::MmsMessage(..., aAttachments), some elements of |aAttachments| will be unexpectedly changed (even *before* doing mAttachments(aAttachments)).

For example, the |attachments| is an array of 4 MmsAttachment elements. The content of 2nd and 3rd elements (like .id or .location) printed in the constructor doesn't equal to the one before passed into the constructor.

This is super weird because the constructor simply eats reference:

  const nsTArray<MmsAttachment>&  aAttachments

which should have no impact at all! It's totally beyond my knowledge but my patch indeed work. Could you please take this review? Thanks!

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 727137 [details] [diff] [review]
Patch

Yikes, I didn't realize that we used nsAutoString. We should probably move to nsString for dictionaries (even though it might be a bit slower).

I'm going to clear the review request here for now. If we can't change the dictionary stuff, I'd be OK with this to get it in, but I'd like to investigate more correct solutions first.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Does it help if http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/dictionary_helper_gen.py?rev=d6766dee457e&force=1&mark=58-58#53
is changed from nsAutoString to nsString?

I'm not sure whether it would be actually slower, and dictionaries aren't, AFAIK, used in
super hot code paths.

if nsAutoString -> nsString works, r=smaug for such change :)

Comment 4

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2 (checked-in)

Hi Blake and Smaug,

s/nsAutoString/nsString works for me. :D Thanks for your investigation and suggestion. I'm waiting on the try results before landing this with r=smaug:

https://tbpl.mozilla.org/?tree=Try&rev=1797d161042f

As my best understanding, there should be no risk to s/nsAutoString/nsString, because the only difference between them is the internal way of allocating memory. Right?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Gene Lian [:gene] from comment 4)
> As my best understanding, there should be no risk to
> s/nsAutoString/nsString, because the only difference between them is the
> internal way of allocating memory. Right?

Yep. nsAutoString contains a buffer in the object that serves as the storage for short strings whereas nsString always uses malloc (for strings that don't fit in the internal buffer nsAutoString switches to malloc). The problem with nsTArray is that it moves objects, so strings with pointers that point to their internal buffers end up with pointers pointing to the old location of the object.

Comment 6

[Gene Lian (I already quit Mozilla)]

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

Comment 7

[Ed Morley [:emorley]]

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

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Please go ahead and nominate for uplift to v1.1

Comment 9

[Gene Lian (I already quit Mozilla)]

(In reply to [e-mail] from comment #8)
> Please go ahead and nominate for uplift to v1.1

Wait! Do we already have a branch for v1.1? My understanding is we can use either of the following 2 types flags (depending if the bug is critical):

  - leo+
  - approval-mozilla-b2g18+

to uplift patches to the mozilla-b2g18 branch. On Apr 26, then we'll create v1.1 on the tip of mozilla-b2g18. So far, we don't need to uplift patches to the v1.1. Correct?

Comment 10

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

(In reply to [e-mail] from comment #8)
> Please go ahead and nominate for uplift to v1.1

Why should we ever nominate a leo- bug for v1.1 uplift? Are you really sure what you're doing?

Comment 11

[Gene Lian (I already quit Mozilla)]

Comment on attachment 727502 [details] [diff] [review]
Patch, V2 (checked-in)

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 #): bug 844431
User impact if declined: without this, the nsIDOMMozMmsMessage.attachments cannot be correctly exposed to the content, which means the Gaia cannot see the correct attachments of a received MMS.
Testing completed: yes
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: no

Note: something bad happens if we put a dictionary structure containing nsAutoString into an nsTArray. We have to use nsString instead to avoid the wrong use. This patch should be safe because nsAutoString and nsString have no difference when doing the String operations (except for its internal way of allocating memory). Please ping Blake or Smaug if you'd still worry about that.

Comment 12

[Gene Lian (I already quit Mozilla)]

(In reply to Gene Lian [:gene] from comment #11)
> Note: something bad happens if we put a dictionary structure containing
> nsAutoString into an nsTArray. We have to use nsString instead to avoid the
> wrong use. This patch should be safe because nsAutoString and nsString have
> no difference when doing the String operations (except for its internal way
> of allocating memory). Please ping Blake or Smaug if you'd still worry about
> that.

Please see comment #5 for more technical details.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 727502 [details] [diff] [review]
Patch, V2 (checked-in)

Gene - comment 9 is correct and sorry if it wasn't clear when I said "please nominate for uplift to v1.1" I should have said "please nominate for approval-mozilla-b2g18" which is currently the same thing as we have not branched the mozilla-b2g18 repo to a v1.1 repo yet.

Approving this low risk landing to get uplifted to mozilla-b2g18 and bake there. With regards to the nsString vs. nsAutoString - is this something we need to ask QA to poke at to ensure no regressions?  It's not clear what the concern would be due to that choice - does memory allocation affect performance?

Comment 14

[Gene Lian (I already quit Mozilla)]

Attachment: [b2g18] Patch

Comment 15

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5e030b84782c

Comment 16

[Gene Lian (I already quit Mozilla)]

Added [NO_UPLIFT] per recent commercial RIL compatibility issue. Waiting on further decision to keep the patch in b2g18 or to back it out.

------------------------------
If we really want to back them out, backing the following MMS bugs should be enough to make the commercial RIL compatible:

Bug 854422 - B2G MMS: should call .NotifyResponseTransaction() with MMS_PDU_STATUS_RETRIEVED after an MMS is retrieved under the RETRIEVAL_MODE_AUTOMATIC mode (a follow-up for bug 845643)
Bug 850680 - B2G MMS: broadcast "sms-received" and "sms-sent" system messages
Bug 850530 - B2G MMS: Use the same attribute name for delivery (s/state/delivery) like SMS
Bug 852911 - B2G MMS: fail to expose correct nsIDOMMozMmsMessage.attachments.
Bug 853725 - B2G MMS: fail to read nsIDOMMozMmsMessage.receivers for a received MMS (a follow-up of bug 849741).
Bug 853329 - B2G MMS: other Android phones cannot read attachments sent from FFOS
Bug 852471 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first) (follow-up fix)
Bug 852460 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event (follow-up fix)
Bug 849741 - B2G MMS: provide nsIDOMMobileMessageManager.onreceived event
Bug 847756 - B2G MMS: provide nsIDOMMobileMessageManager.markMessageRead().
Bug 847736 - B2G MMS: provide nsIDOMMobileMessageManager.delete().
Bug 847738 - B2G MMS: provide nsIDOMMobileMessageManager.getMessage(). 
Bug 844431 - B2G MMS: provide nsIDOMMobileMessageManager interface (with sendMMS() first)
Bug 845643 - B2G MMS: Save retrieved MMS into database.

Comment 17

[Gene Lian (I already quit Mozilla)]

Following the previous comment, some more needs to back out:

Bug 792321 - Check max values of MMS parameters in sendRequest.
Bug 833291 - B2G SMS & MMS: getMessages it's not working with PhoneNumberJS
Bug 844429 - B2G SMS & MMS: move SMS codes into dom/mobilemessage to make it generic for MMS
Bug 839436 - B2G MMS: make DB be able to save MMS messages.

Comment 18

[Gene Lian (I already quit Mozilla)]

Confirming with Michael to see we should back out to Bug 839436 or just Bug 844431 (if we eventually decide to back out). Please see Bug 857632, comment #17.

Comment 19

[Gene Lian (I already quit Mozilla)]

Per off-line discussion with Michael, we decided not to back out the MMS bugs that have already been in mozilla-b2g18. Removing [NO_UPLIFT] to make the check-in status sync'ed.