B2G MMS: Change MmsService to transaction-based

RESOLVED FIXED in Firefox 22

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: ctai, Assigned: ctai)

Tracking

unspecified
mozilla22
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:leo+, firefox20 wontfix, firefox21 wontfix, firefox22 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix)

Details

Attachments

(2 attachments, 22 obsolete attachments)

50.01 KB, patch
Details | Diff | Splinter Review
81.74 KB, patch
Details | Diff | Splinter Review
Comment hidden (empty)
Created attachment 708496 [details] [diff] [review]
WIP-almost done
Attachment #706998 - Attachment is obsolete: true
Created attachment 708498 [details] [diff] [review]
WIP-almost done
Attachment #708496 - Attachment is obsolete: true
Created attachment 708499 [details] [diff] [review]
WIP-almost done
Attachment #708498 - Attachment is obsolete: true
Created attachment 708500 [details] [diff] [review]
WIP-almost done
Attachment #708499 - Attachment is obsolete: true
Created attachment 708914 [details] [diff] [review]
Txn-based MMS
Attachment #708500 - Attachment is obsolete: true
Created attachment 708917 [details] [diff] [review]
Txn-based MMS
Attachment #708914 - Attachment is obsolete: true
Attachment #708917 - Flags: review?(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.
Created attachment 708994 [details] [diff] [review]
Txn-based MMS
Attachment #708917 - Attachment is obsolete: true
Attachment #708917 - Flags: review?(vyang)
Created attachment 708995 [details] [diff] [review]
Txn-based MMS
Attachment #708994 - Attachment is obsolete: true
Attachment #708995 - Flags: review?(vyang)
Created attachment 709591 [details] [diff] [review]
Txn-based MMS
Attachment #708995 - Attachment is obsolete: true
Attachment #708995 - Flags: review?(vyang)
Attachment #709591 - Flags: review?(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.
Attachment #709591 - Flags: review?(vyang)
Created attachment 711226 [details] [diff] [review]
WIP-0207
Attachment #709591 - Attachment is obsolete: true
Created attachment 715032 [details] [diff] [review]
Txn-based MMS
Attachment #711226 - Attachment is obsolete: true
Created attachment 715033 [details] [diff] [review]
Txn-based MMS
Attachment #715032 - Attachment is obsolete: true
Attachment #715033 - Flags: feedback?(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.
Attachment #715033 - Flags: feedback?(vyang)
Created attachment 715972 [details] [diff] [review]
Txn-based MMS
Attachment #715033 - Attachment is obsolete: true
Created attachment 715975 [details] [diff] [review]
Txn-based MMS
Attachment #715972 - Attachment is obsolete: true
Attachment #715975 - Flags: review?(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.
Attachment #715975 - Flags: review?(vyang)
Created attachment 716365 [details] [diff] [review]
Txn-based MMS
Attachment #715975 - Attachment is obsolete: true
Attachment #716365 - Flags: review?(vyang)
Created attachment 716373 [details] [diff] [review]
Txn-based MMS
Attachment #716365 - Attachment is obsolete: true
Attachment #716365 - Flags: review?(vyang)
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
Created attachment 716433 [details] [diff] [review]
Txn-based MMS

^o^
Attachment #716373 - Attachment is obsolete: true
Created attachment 716436 [details] [diff] [review]
Txn-based MMS

\^o^/
Attachment #716433 - Attachment is obsolete: true
Created attachment 716437 [details] [diff] [review]
Txn-based MMS

\^o^/
Attachment #716436 - Attachment is obsolete: true
Attachment #716437 - Flags: review?(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.
Attachment #716437 - Flags: review?(vyang) → review+
Created attachment 716441 [details] [diff] [review]
Txn-based MMS
Attachment #716437 - Attachment is obsolete: true
Keywords: checkin-needed
Thanks for your great help. I learned a lot from the process. :)
(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 :)
https://hg.mozilla.org/mozilla-central/rev/f8dd15944b33
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Duplicate of this bug: 827789
blocking-b2g: --- → leo?
leo? for it blocks bug 810067.
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
Leo triage: leo+ for MMS blockers
blocking-b2g: leo? → leo+
Looks like this will need a b2g18-specific patch for uplift.
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.
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.
Created attachment 722028 [details] [diff] [review]
[b2g18]-012-bug-833697-fix
I have already uploaded all related b2g18 specific patchs for this bug.
Thanks.
Created attachment 722100 [details] [diff] [review]
[b2g18]-012-bug-833697-fix

Adding a=leo+.
Attachment #722028 - Attachment is obsolete: true
https://hg.mozilla.org/releases/mozilla-b2g18/rev/3336c7f59e49
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-firefox22: --- → fixed
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.
status-b2g18: fixed → affected
status-firefox20: --- → wontfix
status-firefox21: --- → wontfix
(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.
(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.
Flags: needinfo?(ryanvm)
status-b2g18: affected → fixed
Flags: needinfo?(ryanvm)
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.