833697 - B2G MMS: Change MmsService to transaction-based

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

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

Attachment: WIP

Comment 2

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

Attachment: WIP-almost done

Comment 3

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

Attachment: WIP-almost done

Comment 4

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

Attachment: WIP-almost done

Comment 5

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

Attachment: WIP-almost done

Comment 6

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

Attachment: Txn-based MMS

Comment 7

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

Attachment: Txn-based MMS

Comment 8

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

Comment on attachment 708917 [details] [diff] [review]
Txn-based MMS

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

::: dom/mms/src/ril/MmsService.js
@@ +50,5 @@
>    return Cc["@mozilla.org/telephony/system-worker-manager;1"].
>             getService(Ci.nsIInterfaceRequestor).
>             getInterface(Ci.nsIRadioInterfaceLayer);
>  });
> +/**

New line, please.

@@ +72,5 @@
> +}
> +Transaction.prototype = {
> +  applyFilter: function applyFilter(service, uri, proxyInfo) {
> +    let url = uri.prePath + uri.path;
> +    url = url.replace(/\/$/, "");

Can we have following instead?

  if (url.endsWith("/")) {
    url = url.substr(0, url.length - 1);
  }

@@ +83,5 @@
> +          JSON.stringify({ contentUri: this.contentUri,
> +                           mmsProxyInfo: this.mms.mmsProxyInfo }));
> +    return this.mms.mmsProxyInfo;
> +  },
> +  // Abstract function.

New line.

@@ +98,5 @@
> +    let istream = this.istream;
> +    let callback = this.callback;
> +
> +    let mms = this.mms;
> +    if (!mms.acquireMmsConnection(this)) {

Can we move connection management code into another object? Somewhere neither Transaction nor MmsService, for example, MmsConnection? Then we have MmsConnection.acquire(), .release() instead. All these connection things should be moved into NetworkManager someday.

Comment 9

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

Attachment: Txn-based MMS

Comment 10

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

Attachment: Txn-based MMS

Comment 11

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

Attachment: Txn-based MMS

Comment 12

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

Comment on attachment 709591 [details] [diff] [review]
Txn-based MMS

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

::: dom/mms/src/ril/MmsService.js
@@ +206,5 @@
> +});
> +
> +/**
> + * NotificationResponseTransaction.
> + *   Class for send M-NotifyResp.ind back to MMSC.

sending.

@@ +228,5 @@
> +  // override
> +  process: {
> +    value: function process() {
> +      this.istream = MMS.PduHelper.compose(null, {headers: this.headers});
> +      Transaction.prototype.sendMmsRequest.apply(this);

this.sendMmsRequest()?

@@ +269,5 @@
> +
> +      let that = this;
> +      let callback = function callback(istream) {
> +        that.istream = istream;
> +        Transaction.prototype.sendMmsRequest.apply(that);

Please move this object near the compose() call.  The name 'callback' duplicates 'this.callback' as well.  Please rename to something more meaningful, or even make it an line function.

@@ +419,5 @@
> +    // If the MMS network is not yet connected, buffer the
> +    // MMS request and try to setup the MMS network first.
> +    if (!this.mmsNetworkConnected) {
> +      debug("acquire: " +
> +            "buffer the MMS request and setup the MMS data call.");

Move to the same line.

@@ +685,5 @@
>          wish = retr.headers["x-mms-delivery-report"];
>        }
>        let ra = this.getReportAllowed(this.confSendDeliveryReport, wish);
>  
> +      let txn = new NotificationResponseTransaction(this.mmsConn, this.mmsConn.mmsc, null, null);

The reason we want a transaction based model is we can move some transaction specific code into a certain object class and keep MMS service clean.  MMS service will then only handle events or requests between database and transactions.  So please move everything related to mms connection out, and move everything related to a certain transaction to its constructor.  Pass only necessary parameters in order to create a transaction.  For example, NotificationResponseTransaction's constructor should accept only |tid| & |status|.  Please move |ra| into its constructor as well.

The Transaction.process() should accept a callback function, not the constructor.  An action can have a callback, but a constructor don't.  The parseStreamAndDispatch() should be dead now.  The process flow should be hidden in transaction callbacks.

@@ +788,5 @@
>          }, this);
> +        this.mmsConn.timerToClearQueue.cancel();
> +        this.mmsConn.timerToClearQueueCb();
> +        this.mmsConn.timerToReleaseMmsConnection.cancel();
> +        this.mmsConn.timerToReleaseMmsConnectionCb();

Please move all these into MmsConnection.observe() as well.

::: dom/mms/src/ril/WspPduHelper.jsm
@@ +2453,4 @@
>  
> +      // Append part content and header.
> +      if (part.content.length) {
> +    	// Append per-part header

Tab.

Comment 13

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

Attachment: WIP-0207

Comment 14

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

Attachment: Txn-based MMS

Comment 15

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

Attachment: Txn-based MMS

Comment 16

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

Comment on attachment 715033 [details] [diff] [review]
Txn-based MMS

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

::: dom/mms/src/ril/MmsService.js
@@ +115,5 @@
> +      this.clearMmsProxySettings();
> +    }
> +
> +    try {
> +      this.urlUAProf = Services.prefs.getCharPref('wap.UAProf.url');

UAProf stuff should belong to Transaction because they're only referenced there.

@@ +426,5 @@
> +/**
> + * NotificationTransaction.
> + *   Class for response incoming M-Notification.ind.
> + */
> +function NotificationTransaction(mms, contentUri, msg) {

Please pass only necessary parameters to FooTransaction.  For NotificationTransaction, it's content location url[1].  Construct msg object in this constructor.

[1]: There are actually other mandatory fields listed in MMS-ENC, but content location is sufficient for our need now.

@@ +456,5 @@
> +      // Mandatory fields
> +      notifyRespondMsg.headers["x-mms-transaction-id"] = this.msg.headers["x-mms-transaction-id"];
> +      notifyRespondMsg.headers["x-mms-status"] = status;
> +      // Optional fields
> +      notifyRespondMsg.headers["x-mms-report-allowed"] = this.mms.getReportAllowed(this.mms.confSendDeliveryReport, wish);

Please move these code pieces to ctor of NotificationResponseTransaction, passing transaction id & status only. X-Mms-Report-Allowed should be calculated in ctor of NotificationResponseTransaction, too.

@@ +482,5 @@
> +        this.notifiResponeCallabck(MMS.MMS_PDU_ERROR_TRANSIENT_FAILURE, null);
> +      } else if (!data) {
> +        this.notifiResponeCallabck(MMS.MMS_PDU_STATUS_DEFERRED, null);
> +      }
> +     let parsedMsg = MMS.PduHelper.parse(data, null);

Indentations ...

@@ +500,5 @@
> +  // override
> +  // @see OMA-TS-MMS_ENC-V1_3-20110913-A section 6.2
> +  process: {
> +    value: function process(callback) {
> +      this.sendMmsRequest(this.sendReqCallback.bind(this));

'callback' is never used.

@@ +750,5 @@
>     *
>     * @return true if incoming data parsed successfully and passed to PDU
>     *         handler; false otherwise.
>     */
> +  parseStream: function parseStream(data, options) {

parseStream should be dead now.

Comment 17

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

Attachment: Txn-based MMS

Comment 18

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

Attachment: Txn-based MMS

Comment 19

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

Comment on attachment 715975 [details] [diff] [review]
Txn-based MMS

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

::: dom/mms/src/ril/MmsService.js
@@ +280,5 @@
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIProtocolProxyFilter]),
> +
> +  // nsIProtocolProxyFilter
> +
> +  applyFilter: function applyFilter(service, uri, proxyInfo) {

Maybe rename |service| to |proxyService| to clarify it's not mmsService?

@@ +286,5 @@
> +    if (url.endsWith("/")) {
> +      url = url.substr(0, url.length - 1);
> +    }
> +
> +    if (this.contentLocation != url) {

Suddenly find this may cause another bug: we can't have two simultaneous http requests in one transaction. But do we need it at all? Probably not.

@@ +423,5 @@
> + * @see OMA-TS-MMS_ENC-V1_3-20110913-A section 6.2
> + */
> +function NotifyResponseTransaction(transactionId, status, reportAllowed) {
> +  Transaction.call(this);
> +  this.contentLocation = gMmsConnection.mmsc;

This line should be in |Transaction.sendRequest()| so that it does all necessary preparing before committing http requests.

@@ +468,5 @@
> + */
> +function RetrieveTransaction(contentLocation) {
> +  Transaction.call(this);
> +
> +  this.contentLocation = contentLocation;

Ditto.

@@ +515,5 @@
> + *   Class for sending M-Send.req to MMSC
> + */
> +function SendTransaction(msg) {
> +  Transaction.call(this);
> +  this.contentLocation = gMmsConnection.mmsc;

Ditto.

@@ +589,5 @@
> +  /**
> +   * @param callback [optional]
> +   *        A callback function that takes zero argument.
> +   */
> +  converPartsToUi8a: {

Never get called?

::: dom/mms/src/ril/WspPduHelper.jsm
@@ +2418,5 @@
>        // Encode headersLen, DataLen
>        let headersLen = data.offset;
>        UintVar.encode(data, headersLen);
> +
> +      UintVar.encode(data, part.ui8a.length);

I'd prefer its original name though.

Comment 20

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

Attachment: Txn-based MMS

Comment 21

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

Try server result:
https://tbpl.mozilla.org/?tree=Try&rev=d12a94d77e6b

Comment 22

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

Attachment: Txn-based MMS

Comment 23

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

Fix the patch message information....
(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #22)
> Created attachment 716373 [details] [diff] [review]
> Txn-based MMS

Comment 24

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

Attachment: Txn-based MMS

^o^

Comment 25

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

Attachment: Txn-based MMS

\^o^/

Comment 26

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

Attachment: Txn-based MMS

\^o^/

Comment 27

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

Comment on attachment 716437 [details] [diff] [review]
Txn-based MMS

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

r=me with some nits fixed. Thank you for this great job :)

::: dom/mms/src/ril/MmsService.js
@@ +608,5 @@
> +      return;
> +    }
> +
> +    let numPartsToLoad = parts.length;
> +    for each(let part in parts) {

Sorry, but please have a space between 'each' and left parenthesis.

@@ +617,5 @@
> +          return;
> +        }
> +        continue;
> +      }
> +      debug("part.content.size = " + part.content.size);

Looks like a personal debug message?

@@ +621,5 @@
> +      debug("part.content.size = " + part.content.size);
> +      let fileReader = Cc["@mozilla.org/files/filereader;1"]
> +                       .createInstance(Ci.nsIDOMFileReader);
> +      fileReader.addEventListener("loadend",
> +              (function onloadend(part, event) {

Alignment.

@@ +673,5 @@
> +      }
> +
> +      let response = MMS.PduHelper.parse(data, null);
> +      if (!response || (response.type != MMS.MMS_PDU_TYPE_SEND_CONF)) {
> +          callbackIfValid(MMS.MMS_PDU_RESPONSE_ERROR_UNSUPPORTED_MESSAGE, null);

Alignment.

::: dom/mms/src/ril/WspPduHelper.jsm
@@ +2427,5 @@
> +      if (typeof part.content === "string") {
> +        let charset;
> +        if (contentType && contentType.params && contentType.params.charset &&
> +           contentType.params.charset.charset) {
> +          charset = contentType.params.charset.charset;

Alignment.

Comment 28

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

Attachment: Txn-based MMS

Comment 29

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

Thanks for your great help. I learned a lot from the process. :)

Comment 30

[Gene Lian (I already quit Mozilla)]

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

Comment 31

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

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #29)
> Thanks for your great help. I learned a lot from the process. :)

Me too :)

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

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

leo? for it blocks bug 810067.

Comment 35

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

The previous bugs landed sequence:
788441	SMS: RangeError in semiOctetToBcdChar	Landed in 2012-11-13
792330	Support multiple media types 	Landed in 2012-11-22
804754	B2G MMS: support UAProfile in HTTP header	Landed in 2012-11-29
815020	 Can not call from WapPushManager.processMessage to MmsService.receiveWapPush	Landed in 2012-11-29
823816	 B2G MMS: WSP Text decode check the wrong range of ASCII code.	Landed in 2012-12-26
825836	Support Blob in JS components	Landed in 2013-01-08
801632	B2G MMS: use Blob for attachment raw data 	Landed in 2013-01-10
792328	 B2G MMS: MMSCONF-MED-C-00{1,2,3,4,5}: Support for {us-ascii,utf-8,utf-16,iso-8859-1} as media type text	Landed in 2013-01-20

Comment 36

[Dylan Oliver [:doliver]]

Leo triage: leo+ for MMS blockers

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Looks like this will need a b2g18-specific patch for uplift.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

I'd like to get this uplifted (along with some of the other leo+ bugs), but it doesn't apply cleanly enough to b2g18 for me to comfortably do so. Please post a branch-specific patch or get branch approval for any dependent bugs.

Comment 39

[Gene Lian (I already quit Mozilla)]

Between bug 801632 and bug 825836 we also need to uplift the following bugs, which are all labeled as leo+ already:

2013-01-04 16:21 +0800	Gene Lian - Bug 820733 - B2G MMS: Don't deactivate the MMS network so often which is a burden for consecutive MMS HTTP requests. r=vicamo
diff

2012-12-13 14:24 +0800	Gene Lian - Bug 817474 - B2G MMS: handleNotificationIndication()->sendMmsRequest() cannot get the expected HTTP response (part 2). r=vicamo

2012-12-06 19:48 +0800	Gene Lian - Bug 817474 - B2G MMS: handleNotificationIndication()->sendMmsRequest() cannot get the expected HTTP response (part 1). r=vicamo

2012-11-30 18:09 +0800	Gene Lian - Bug 777251 - B2G MMS: Configure MMS proxy settings through SettingsService. r=vicamo(In reply to Ryan VanderMeulen [:RyanVM] from comment #38)

> I'd like to get this uplifted (along with some of the other leo+ bugs), but
> it doesn't apply cleanly enough to b2g18 for me to comfortably do so. Please
> post a branch-specific patch or get branch approval for any dependent bugs.

Yes... the MMS codes are really out-of-date between m-c and b2g-18, which makes it difficult to sync.

Comment 40

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

Attachment: [b2g18]-012-bug-833697-fix

Comment 41

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

The order is below...
https://bugzilla.mozilla.org/show_bug.cgi?id=792330
https://bugzilla.mozilla.org/show_bug.cgi?id=804754
https://bugzilla.mozilla.org/show_bug.cgi?id=815020
https://bugzilla.mozilla.org/show_bug.cgi?id=823816
https://bugzilla.mozilla.org/show_bug.cgi?id=825836
https://bugzilla.mozilla.org/show_bug.cgi?id=777251
https://bugzilla.mozilla.org/show_bug.cgi?id=817474
https://bugzilla.mozilla.org/show_bug.cgi?id=820733
https://bugzilla.mozilla.org/show_bug.cgi?id=801632
https://bugzilla.mozilla.org/show_bug.cgi?id=792328
https://bugzilla.mozilla.org/show_bug.cgi?id=833697

Comment 42

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

I have already uploaded all related b2g18 specific patchs for this bug.
Thanks.

Comment 43

[Gene Lian (I already quit Mozilla)]

Attachment: [b2g18]-012-bug-833697-fix

Adding a=leo+.

Comment 44

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/3336c7f59e49

Comment 45

[Gene Lian (I already quit Mozilla)]

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #41)
> The order is below...
> https://bugzilla.mozilla.org/show_bug.cgi?id=792330
> https://bugzilla.mozilla.org/show_bug.cgi?id=804754
> https://bugzilla.mozilla.org/show_bug.cgi?id=815020
> https://bugzilla.mozilla.org/show_bug.cgi?id=823816
> https://bugzilla.mozilla.org/show_bug.cgi?id=825836
> https://bugzilla.mozilla.org/show_bug.cgi?id=777251
> https://bugzilla.mozilla.org/show_bug.cgi?id=817474
> https://bugzilla.mozilla.org/show_bug.cgi?id=820733
> https://bugzilla.mozilla.org/show_bug.cgi?id=801632
> https://bugzilla.mozilla.org/show_bug.cgi?id=792328
> https://bugzilla.mozilla.org/show_bug.cgi?id=833697

The above-mentioned b2g18-specific patches have been carefully landed onto b2g-18. Thanks Chia-hung for all the hard work! :)

Comment 46

[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 47

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

Attachment: [b2g18]-bug-833697

https://tbpl.mozilla.org/?tree=Try&rev=8baf1daa77fa

Comment 48

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

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #47)
> Created attachment 722660 [details] [diff] [review]
> [b2g18]-bug-833697
> 
> https://tbpl.mozilla.org/?tree=Try&rev=8baf1daa77fa

B2G Arm (VM) opt 1 2 3 4 5 6 7 8 9 MnX is passed.

Comment 49

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

(In reply to Chia-hung Tai [:ctai :ctai_mozilla :cht] from comment #47)
> Created attachment 722660 [details] [diff] [review]
> [b2g18]-bug-833697
> 
> https://tbpl.mozilla.org/?tree=Try&rev=8baf1daa77fa

This patch look likes OK in B2G Mn on b2g18 branch.

Comment 50

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

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