Vicamo Yang [:vicamo][:vyang] Assignee
	Description • 12 years ago

      No description provided.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 1 • 12 years ago

WIP branch https://github.com/vicamo/b2g_releases-mozilla-central/tree/bugzilla/838467/domcursor-for-sms

	Gene Lian (I already quit Mozilla)
	Comment 2 • 12 years ago

This bug is also a pre-condition of

  - bug 847760 - B2G MMS: provide nsIDOMMobileMessageManager.getMessages()
  - bug 849739 - B2G MMS: provide nsIDOMMobileMessageManager.getThreadList()

to support the MMS DOM APIs. Needs to be marked as leo+.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 3 • 12 years ago

In this patch I remove SmsCursor completely and re-implement original functionality with re-usable classes. Gaia fixes is required.

Changes summary:

1) nsIDOMMozSmsManager::getMessages returns nsIDOMDOMCursor instead.

2) Create a new interface nsIMobileMessageCursorCallback. This interface collects cursor related methods originally available in nsISmsRequest. Like nsISmsRequest, it's passed as an argument to nsIMobileMessageDatabaseService::createMessageCursor. MobileMessageCursorCallback implements nsIMobileMessageCursorCallback.

3) nsIMobileMessageDatabaseService::createMessageList is renamed to "createMessageCursor". It accepts the same arguments as before except the last argument "nsISmsRequest" is replaced by nsIMobileMessageCursorCallback. This method also returns an object that implements nsICursorContinueCallback, which is a constructor parameter of DOMCursor.

4) For IPC changes, now mobile message cursor has its own protocol, named PMobileMessageCursor, which is basically extracted from PSmsRequest. The original SmsCursor IPC has to create a PSmsRequest child/parent pair each time you make a call to continue(). Now, the life time of PMobileMessageCursor is the whole session span of a DOMCursor operation. MobileMessageCursorChild implements nsICursorContinueCallback, and MobileMessageCursorParent implements nsIMobileMessageCursorCallback.

5) By the design of DOMCursor, we either create a callback class[1] or sub-class DOMCursor itself[2]. However, sub-classing a DOM component with extra internal interface implemented and passing it to a XPCOM service written in javascript causes the problem we had in bug 775997 comment #51.

6) For reference owning, DOMCursor owns a MobileMessageCursorChild, which owns a MobileMessageCursorCallback, which owns the DOMCursor. MobileMessageCursorParent and the returned object from nsIMobileMessageDatabaseService::CreateMessageCursor own each other. Whenever any of these objects is notified with DONE or ERROR events, it gives up owned reference to others immediately.

[1]: see bug 837917
[2]: see WIP patch in bug 837865

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 6 • 12 years ago

Refactor filter handling code and separate them into several smaller objects. Preparing for bug 771458, bug 771463 and bug 849739.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 8 • 12 years ago

a few minor fixes.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 9 • 12 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> In this patch I remove SmsCursor completely and re-implement original
> functionality with re-usable classes. Gaia fixes is required.

See bug 849739 attachment #724528 [details] [diff] [review] about how the architecture is re-used.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 10 • 12 years ago

Is there really any point in spending time doing this? I feel like there are more important things to spend time on right now. It's easy to change the SMSCursor API to just return nsISupports instead of nsIDOMSMSMessage so that we can reuse it for MMS.

I realize that the patches have already been written and so it's a sunk cost, but there's still a lot of work remaining on reviewing and fixing review comments.

I'm sorry if it's something I've said to feel the need to make this change. That was really not my intent.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 11 • 12 years ago

(In reply to Jonas Sicking (:sicking) from comment #10)

Just get it landed ok?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 12 • 12 years ago

Vicamo, I don't understand what you mean by the above comment.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 13 • 12 years ago

Rebased to bug 844431. Patches on the way.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 14 • 12 years ago

Fix MobileMessageManager as well.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 15 • 12 years ago

1) fix build errors on other platforms
2) rebase and fix MobileMessageManager as well

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 16 • 12 years ago

Rebase only

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 17 • 12 years ago

Rebase only.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 18 • 12 years ago

Rebase only.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 19 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=10fad2c75bcb

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 20 • 12 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #19)
> https://tbpl.mozilla.org/?tree=Try&rev=10fad2c75bcb

Breaks test_sms_basics.html, will fix later.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 21 • 12 years ago

One of many new features of MMS that makes it so different from SMS is MMS supports multiple recipients in one message. However we don't have a correct API to retrieve a list of messages in the same thread in multi-recipient world. The method we're using now is basically a abuse of SmsFilte::numbers, and it just can't go any further.

The ultimate solution is thread id, which is already implemented in database in bug 833291. The first thing we have to do is make GetThreadList return something containing thread id. This step is now in bug 849739 and I've just uploaded patches and test cases for it.

The second step is exposing thread id in SmsMessage and MmsMessage. I've already upgrade database schema for this in, again, bug 833291. Having thread id in message objects is for matching a just received message with current conversation view. So our Gaia app doesn't have to query database, update messages list and redraw the view. See bug 850127 for patches.

The third, also the last, part of the plan is to add an extra filter condition, thread id of course. This way we have finally a API for retrieving thread messages that does mean it. It should be bug 847760.

Another big trouble we have is bug 824467, GetThreadList is broken in non-oop mode. And we don't have OOP for MMS yet. To enable Gaia development right away, rewrite GetThreadList becomes a must. It's now fixed in bug 849739, too. With the foundation made in this bug, we have a common, shared way for two cursor based APIs. And that's not some paper drawing because the patches for all of these things are already listed above and are waiting for your review. So please just don't think about other easy ways, let's get things done in the right and complete way.

	Dietrich Ayala (:dietrich)
	Comment 22 • 12 years ago

Removing blocking request. There's not enough information for the triage group to make a blocking call. Vicamo and Jonas, can you please meet and figure this out, and re-nominate if necessary?

	Ken Chang[:kenkai]
	Comment 23 • 12 years ago

Jonas is helping us to review this bug. Thanks for Jonas' help. 
And this bug blocks some Leo+ bugs. I think this bugs should be marked as Leo+

	Joe Cheng [:jcheng] (please needinfo)
	Comment 24 • 12 years ago

talked to RIL team, base on the current approach on MMS gecko, this is needed to support the getThreadList API, which is blocking the display of MMS messages in Gaia.
This needs to land.

	Alex Keybl [:akeybl]
	Comment 25 • 12 years ago

Blocking a blocker.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 26 • 12 years ago

Comment on attachment 724857 [details] [diff] [review]
Part 1/5: interface changes : v2

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

r=me, but it would be good to get rid of the new interface since I don't think it's needed.

::: dom/mobilemessage/interfaces/nsIMobileMessageCursorCallback.idl
@@ +8,5 @@
> +interface nsIMobileMessageCursorCallback : nsISupports
> +{
> +  void notifyCursorError(in long error);
> +  void notifyCursorResult(in nsISupports result);
> +  void notifyCursorDone();

Do you really need this interface? Can't you use the MobileMessageCursorCallback directly everywhere? I suspect you'll only need the .idl interface if you are using this interface directly from JS.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 27 • 12 years ago

Comment on attachment 724858 [details] [diff] [review]
Part 2/5: DOM : v2

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

In general the setup seems good though I'm not 100% sure I follow the flow exactly.

My main concern is that we're loading each message when we're getting the .continue() call from the cursor. There's a risk that this will have performance issues since each SMS message will do a pretty long round-trip from child->IPC->parent->IDB thread->parent->IPC->child. In the contacts API we saw some very big performance improvements by reading ahead in the parent and then caching results in the child. So when there's a call to .continue() we simply loaded data from a local array most of the time and were able to just do a single roundtrip to the event loop on the main thread in the child.

That can be done as a followup of course since that's a bigger change than what you're doing here. But it's something we should look at if we want better performance in the SMS app.

The comments below are pretty simple but I'd like to see another patch. I need to take one more look to fully understand the flow here anyway.

::: dom/mobilemessage/src/MobileMessageCursorCallback.cpp
@@ +25,5 @@
> +NS_IMPL_CYCLE_COLLECTING_RELEASE(MobileMessageCursorCallback)
> +
> +MobileMessageCursorCallback::MobileMessageCursorCallback()
> +{
> +  MOZ_COUNT_CTOR(MobileMessageCursorCallback);

Just inline the ctor and dtor since they compile away in release builds.

@@ +48,5 @@
> +  NS_ENSURE_TRUE(rs, NS_ERROR_FAILURE);
> +
> +  switch (aError) {
> +    case nsIMobileMessageCallback::NO_SIGNAL_ERROR:
> +      return rs->FireError(cursor, NS_LITERAL_STRING("NoSignalError"));

There's no need to use the request-service from C++. You can simply call cursor->FireError(NS_LITERAL_STRING("NoSignalError")) etc.

@@ +57,5 @@
> +    case nsIMobileMessageCallback::INTERNAL_ERROR:
> +      return rs->FireError(cursor, NS_LITERAL_STRING("InternalError"));
> +    default: // SUCCESS_NO_ERROR is handled above.
> +      MOZ_ASSERT(false, "Unknown error value.");
> +      return NS_ERROR_FAILURE;

Use MOZ_NOT_REACHED here instead. I forget if that's the exact name of the macro, but it's something like that.

@@ +68,5 @@
> +  NS_ENSURE_STATE(mDOMCursor);
> +
> +  nsCOMPtr<nsIDOMRequestService> rs =
> +    do_GetService(DOMREQUEST_SERVICE_CONTRACTID);
> +  NS_ENSURE_TRUE(rs, NS_ERROR_FAILURE);

Same here, no need to use the requestservice. Simply call functions on the DOMCursor directly.

It goes for all places where you are using the requestservice from C++. You can simply use the relevant classes directly.

@@ +81,5 @@
> +  JSObject* global = sc->GetNativeGlobal();
> +  MOZ_ASSERT(global);
> +
> +  JSAutoRequest ar(cx);
> +  JSAutoCompartment ac(cx, global);

Do you really need to push a context since you are also pushing a request and a compartment here? Either Ben Turner, Blake Kaplan or Bobby Holley should check.

::: dom/mobilemessage/src/MobileMessageManager.cpp
@@ +283,5 @@
> +                                        getter_AddRefs(continueCallback));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  rv = rs->CreateCursor(GetOwner(), continueCallback, aCursor);
> +  NS_ENSURE_SUCCESS(rv, rv);

Just call |new DOMCursor|.

@@ +285,5 @@
> +
> +  rv = rs->CreateCursor(GetOwner(), continueCallback, aCursor);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (*aCursor) {

This should never be possible that it's null, just remove this check. If you are really concerned add a MOZ_ASSERT at the top.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 28 • 12 years ago

Comment on attachment 724860 [details] [diff] [review]
Part 3/5: IPC : v2

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

Bent should definitely look at the ipdl here. I don't feel strong enough on the IPC stuff to verify that the usage here is correct.

::: dom/mobilemessage/src/ipc/SmsChild.cpp
@@ +206,5 @@
> +}
> +
> +MobileMessageCursorChild::~MobileMessageCursorChild()
> +{
> +  MOZ_COUNT_DTOR(MobileMessageCursorChild);

Same here, I think you can just inline the ctor and dtor so that they compile away.

@@ +228,5 @@
> +
> +bool
> +MobileMessageCursorChild::RecvNotifyError(const int32_t& aError)
> +{
> +  NS_ENSURE_TRUE(mCallback, false);

I don't think you need runtime checks since you assert that this is non-null in the ctor. Same in the other functions.

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +374,5 @@
> +
> +bool
> +MobileMessageCursorParent::RecvContinue()
> +{
> +  NS_ENSURE_TRUE(mCallback, false);

I think you can get rid of these runtime checks too. Maybe use asserts if you prefer?

@@ +396,5 @@
> +
> +NS_IMETHODIMP
> +MobileMessageCursorParent::NotifyCursorResult(nsISupports* aResult)
> +{
> +  SmsMessage* message = static_cast<SmsMessage*>(aResult);

This isn't very safe. Is there a reason you couldn't change the type of the argument here from nsISupports* to SmsMessage*? That should work if you get rid of the .idl interface and just use the .h interface directly.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 29 • 12 years ago

(In reply to Jonas Sicking (:sicking) from comment #26)
> Comment on attachment 724857 [details] [diff] [review]
> Part 1/5: interface changes : v2
> -----------------------------------------------------------------
> ::: dom/mobilemessage/interfaces/nsIMobileMessageCursorCallback.idl
> @@ +8,5 @@
> > +interface nsIMobileMessageCursorCallback : nsISupports
> 
> Do you really need this interface? Can't you use the
> MobileMessageCursorCallback directly everywhere? I suspect you'll only need
> the .idl interface if you are using this interface directly from JS.

Exactly, the database service calls its methods through this interface. It's actually playing the same role with nsIMobileMessageCallback, which was renamed from nsISmsRequest.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 30 • 12 years ago

(In reply to Jonas Sicking (:sicking) from comment #28)
> Part 3/5: IPC : v2
> Review of attachment 724860 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/mobilemessage/src/ipc/SmsParent.cpp
> @@ +396,5 @@
> > +
> > +NS_IMETHODIMP
> > +MobileMessageCursorParent::NotifyCursorResult(nsISupports* aResult)
> > +{
> > +  SmsMessage* message = static_cast<SmsMessage*>(aResult);
> 
> This isn't very safe. Is there a reason you couldn't change the type of the
> argument here from nsISupports* to SmsMessage*? That should work if you get
> rid of the .idl interface and just use the .h interface directly.

The aResult must be typed as nsISupports because it might be either a MMS or a SMS message. However, MMS IPC is in bug 847744, so it's safe to cast aResult directly to a SmsMessage.

	Reuben Morais [:reuben]
	Comment 31 • 12 years ago

(In reply to Jonas Sicking (:sicking) from comment #27)
> In the contacts API  we saw some very big performance improvements by reading
> ahead in the parent and then caching results in the child. So when there's a 
> call to .continue() we simply loaded data from a local array most of the time 
> and were able to just do a single roundtrip to the event loop on the main 
> thread in the child.

We are now also seeing another consequence of that approach: the parent process uses all the CPU while sending contacts to the child, causing the UI to be unusable until the entire operation is completed. If SMS/MMS wants to do the same, this should be considered carefully.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 32 • 12 years ago

(In reply to Reuben Morais [:reuben] from comment #31)
> We are now also seeing another consequence of that approach: the parent
> process uses all the CPU while sending contacts to the child, causing the UI
> to be unusable until the entire operation is completed. If SMS/MMS wants to
> do the same, this should be considered carefully.

Thank you, that's really something we should take into consideration before having further refactoring.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 33 • 12 years ago

(In reply to Jonas Sicking (:sicking) from comment #27)
> Part 2/5: DOM : v2
> Review of attachment 724858 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/mobilemessage/src/MobileMessageCursorCallback.cpp
> @@ +81,5 @@
> > +  JSObject* global = sc->GetNativeGlobal();
> > +  MOZ_ASSERT(global);
> > +
> > +  JSAutoRequest ar(cx);
> > +  JSAutoCompartment ac(cx, global);
> 
> Do you really need to push a context since you are also pushing a request
> and a compartment here? Either Ben Turner, Blake Kaplan or Bobby Holley
> should check.

That's actually copied from SmsRequest. Will request for Ben's review later.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 34 • 12 years ago

Carry forward sr+ from :sicking.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 35 • 12 years ago

1) Rebase
2) Skip nsIDOMRequestService
3) Inline ctor/dtor. Note some ctor/dtor implementations can't be inlined because they will require inclusion of some header files for nsRefPtr/nsCOMPtr template instanciation.
4) assert on mDOMCursor instead.
5) MOZ_NOT_REACHED instead.

All review comments in comment #27 are addressed except JS API stuff in MobileMessageCursorCallback.cpp, which requires Bent's review.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 36 • 12 years ago

1) Rebase
2) Rename MobileMessageCursorParent::mCallback to mContinueCallback to clarify it's a nsICursorContinueCallback.
3) Inline ctor/dtor. Note some ctor/dtor implementations can't be inlined because they will require inclusion of some header files for nsRefPtr/nsCOMPtr template instanciation.
4) assert on mContinueCallback instead.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 37 • 12 years ago

Rebase only.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 38 • 12 years ago

Fix mochitest bustage.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 39 • 12 years ago

Comment on attachment 727156 [details] [diff] [review]
Part 4/5: RIL : v3

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

Just passing by....

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +81,5 @@
> +
> +  let subject = headers["subject"];
> +  if (subject == undefined) {
> +    subject = "";
> +  }

let subject = headers["subject"] || "";

@@ +87,5 @@
> +  let smil = "";
> +  let attachments = [];
> +  let parts = aMessageRecord.parts;
> +  if (parts) {
> +    for (let i = 0; i < parts.length; i++) {

let partsLen = parts ? parts.length :0;
for (let i = 0; i < partsLen; i++) {
...

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 40 • 12 years ago

I will try to review this Thursday. Sorry for being slow here :(

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 41 • 12 years ago

Comment on attachment 727156 [details] [diff] [review]
Part 4/5: RIL : v3

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

Hi Vicamo, just a very quick review, not finished yet. Not followed your code yet. :(

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1369,5 @@
>  };
>  
> +let FilterSearcherHelper = {
> +
> +  filterIndex: function filterIndex(index, range, direction, txn, collect) {

Not sure if I've missed something... but according to line#1462 |this.filterIndex("delivery", range, direction, txn, single ? collect : intersectionCollector.newContext());|, 'collect' could be ResultsCollector.collect or IntersectionResultsCollector.collect, right? Why 'collect' here always have 3 parameters?

@@ +1598,5 @@
> +  }
> +};
> +
> +function IntersectionResultsCollector(collect, reverse) {
> +  this.realCollect = collect;

What's 'this.realCollect' for? What's the difference between it and 'this.collect'? I got dazzled while I was trying to trace the code and to distinguish them.

@@ +1641,5 @@
> +        }
> +      }
> +
> +      // It was the last processing context and is no more processing.
> +      return this.realCollect(txn, 0, 0);

Oh, when 'this.realCollect' became reference to IntersectionResultsCollector.collect? What I followed was that it shall be ResultsCollector.collect.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 42 • 12 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #30)
> > This isn't very safe. Is there a reason you couldn't change the type of the
> > argument here from nsISupports* to SmsMessage*? That should work if you get
> > rid of the .idl interface and just use the .h interface directly.
> 
> The aResult must be typed as nsISupports because it might be either a MMS or
> a SMS message. However, MMS IPC is in bug 847744, so it's safe to cast
> aResult directly to a SmsMessage.

I don't understand. How are you planning on dealing with this once there can be both MMS and SMS messages getting here? None of the patches in bug 847744 seems to touch this code currently.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 43 • 12 years ago

Comment on attachment 727152 [details] [diff] [review]
Part 2/5: DOM : v3

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

Looks good!

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 44 • 12 years ago

(In reply to Hsin-Yi Tsai (afk till March 28) [:hsinyi] from comment #41)
> Part 4/5: RIL : v3
> Review of attachment 727156 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
> @@ +1369,5 @@
> >  };
> >  
> > +let FilterSearcherHelper = {
> > +
> > +  filterIndex: function filterIndex(index, range, direction, txn, collect) {
> 
> Not sure if I've missed something... but according to line#1462
> |this.filterIndex("delivery", range, direction, txn, single ? collect :
> intersectionCollector.newContext());|, 'collect' could be
> ResultsCollector.collect or IntersectionResultsCollector.collect, right? Why
> 'collect' here always have 3 parameters?

Collect functions have 3 parameters in genernal case. But some collector just don't need all of them and may discard that last one -- timestamp. That's based on the fact that it expects to be called by the timestamp sorted order of the results.

> @@ +1598,5 @@
> > +  }
> > +};
> > +
> > +function IntersectionResultsCollector(collect, reverse) {
> > +  this.realCollect = collect;
> 
> What's 'this.realCollect' for? What's the difference between it and
> 'this.collect'? I got dazzled while I was trying to trace the code and to
> distinguish them.

We may have cascading ResultsCollector configurations like:

  UnionResultsCollector -> IntersectionResultsCollector -> ResultsCollector
  IntersectionResultsCollector -> UnionResultsCollector -> ResultsCollector
  UnionResultsCollector -> ResultsCollector
  IntersectionResultsCollector -> ResultsCollector
  ResultsCollector

So 'realCollect' is the next collect function in use. Not a good name I think. And needs a few more comments for the design.

ResultsCollector is originally 'onNextMessageInListGot' with message record fetch parts removed. IntersectionResultsCollector is originally named 'onNextMessageInMultiFiltersGot', and UnionResultsCollector is 'onNextMessageInMultiNumbersGot'.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 45 • 12 years ago

Comment on attachment 727156 [details] [diff] [review]
Part 4/5: RIL : v3

Hi Gregor, HsinYi takes her vacation for a few days. Could you help review this patch?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 46 • 12 years ago

Rebase to m-c tip. Only changes in comments.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 47 • 12 years ago

Correct variable namings only. Previously r+ from Jonas.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 48 • 12 years ago

1) correct variable naming
2) move a few code into MobileMessageCursorParent::DoRequest as done in SmsRequestParent.

Previously r+ by Jonas.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 49 • 12 years ago

rebase to m-c tip

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 50 • 12 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #38)
> Created attachment 727157 [details] [diff] [review]
> Part 5/5: test cases : v3

Can be cleanly applied to m-c tip.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 51 • 12 years ago

(In reply to Jonas Sicking (:sicking) from comment #42)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #30)
> > The aResult must be typed as nsISupports because it might be either a MMS or
> > a SMS message. However, MMS IPC is in bug 847744, so it's safe to cast
> > aResult directly to a SmsMessage.
> 
> I don't understand. How are you planning on dealing with this once there can
> be both MMS and SMS messages getting here? None of the patches in bug 847744
> seems to touch this code currently.

In this bug, we have only SmsMessage support; in bug 849739, we have both SmsMessage and MobileMessageThread support (see attachment 728109 [details] [diff] [review]); in bug 847744, we add the third possible type -- MmsMessage[1].

[1]: https://github.com/vicamo/b2g_releases-mozilla-central/commit/7a16f6349bc08e99101c09cb71bad152fcaa6ad1#L6R600

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 52 • 12 years ago

Comment on attachment 728093 [details] [diff] [review]
Part 2/5: DOM : v4

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

I think I'm only supposed to check the MobileMessageCursorCallback files, right? If that's not correct please re-request review and I'll be right on it.

r=me on the MobileMessageCursorCallback files with one small change below.

::: dom/mobilemessage/src/MobileMessageCursorCallback.cpp
@@ +60,5 @@
> +  MOZ_ASSERT(mDOMCursor);
> +
> +  nsresult rv;
> +  nsIScriptContext* sc = mDOMCursor->GetContextForEventHandlers(&rv);
> +  NS_ENSURE_TRUE(sc, NS_ERROR_FAILURE);

You should |NS_ENSURE_SUCCESS(rv, rv)| before you check sc here.

@@ +63,5 @@
> +  nsIScriptContext* sc = mDOMCursor->GetContextForEventHandlers(&rv);
> +  NS_ENSURE_TRUE(sc, NS_ERROR_FAILURE);
> +
> +  AutoPushJSContext cx(sc->GetNativeContext());
> +  MOZ_ASSERT(cx);

Yep, this is fine.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 53 • 12 years ago

(In reply to ben turner [:bent] from comment #52)
> Part 2/5: DOM : v4
> Review of attachment 728093 [details] [diff] [review]:
> -----------------------------------------------------------------
> I think I'm only supposed to check the MobileMessageCursorCallback files,
> right? If that's not correct please re-request review and I'll be right on
> it.

Thank you for your review, will fix nits right away. :)

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 54 • 12 years ago

(In reply to ben turner [:bent] from comment #52)
> ::: dom/mobilemessage/src/MobileMessageCursorCallback.cpp
> @@ +63,5 @@
> +  JSAutoRequest ar(cx);
> +  JSAutoCompartment ac(cx, global);

I think Jonas had concern about the two lines actually. They're not called in MobileMessageCallback::NotifySuccess, but called in SmsRequest::SetSuccessInternal. Which one is more precise for these case?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 55 • 12 years ago

Comment on attachment 728092 [details] [diff] [review]
Part 1/5: interface changes : v4

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

r=me. I have a few nits but one thing below that requires at least a followup.

::: dom/mobilemessage/src/ipc/PMobileMessageCursor.ipdl
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +

Nit: extra newline here.

@@ +13,5 @@
> +namespace mozilla {
> +namespace dom {
> +namespace mobilemessage {
> +
> +protocol PMobileMessageCursor

This will work but it's not as clean as I would like. Ideally it would look more like PIndexedDBCursor. That protocol is much simpler and makes the order of state changes much more obvious. Here, for example, you could deliver one or more Notify*() messages unsolicited or in response to one or more Continue() calls. In other words, there's no real coupling between Continue() and a Notify*(). We could fake it and enforce the same thing here with state machine annotations but it would be best to just tidy this up and enforce a better ordering statically.

I realize time is an issue so I'd be fine doing this in a followup as long as we make sure it gets done.

::: dom/mobilemessage/src/ipc/PSms.ipdl
@@ +83,5 @@
>     */
>    PSmsRequest(IPCSmsRequest request);
>  
> +  /**
> +   * Sent when the child makes an asynchronous cursor to the parent.

How about: "Sent when the child creates a message cursor."

::: dom/mobilemessage/src/ipc/SmsTypes.ipdlh
@@ +34,5 @@
>    uint64_t       timestamp; // ms since epoch.
>    bool           read;
>  };
>  
> +struct SmsFilterData {

Nit: { on its ownline.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 56 • 12 years ago

(In reply to ben turner [:bent] from comment #55)
> Part 1/5: interface changes : v4
> Review of attachment 728092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. I have a few nits but one thing below that requires at least a
> followup.
> 
> ::: dom/mobilemessage/src/ipc/PMobileMessageCursor.ipdl
> @@ +13,5 @@
> > +namespace mozilla {
> > +namespace dom {
> > +namespace mobilemessage {
> > +
> > +protocol PMobileMessageCursor
> 
> This will work but it's not as clean as I would like. Ideally it would look
> more like PIndexedDBCursor. That protocol is much simpler and makes the
> order of state changes much more obvious. Here, for example, you could
> deliver one or more Notify*() messages unsolicited or in response to one or
> more Continue() calls. In other words, there's no real coupling between
> Continue() and a Notify*(). We could fake it and enforce the same thing here
> with state machine annotations but it would be best to just tidy this up and
> enforce a better ordering statically.

PIndexedDBCursor creates PIndexedDBRequest each time Continue() is called, right? If we follow its design in SMS/MMS database, that means we have to create multiple PMobileMessageCursorParent instances during one cursor session. That's something I want to explicitly avoid as stated in comment 3 item 4). Besides, you can't actually FireSuccess twice continuously because DOMRequest::FireSuccess() forbid it[1]. That triggers an assertion.

[1]: http://dxr.mozilla.org/mozilla-central/dom/base/DOMRequest.cpp#l116

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 57 • 12 years ago

Comment on attachment 728094 [details] [diff] [review]
Part 3/5: IPC : v4

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

::: dom/mobilemessage/src/ipc/SmsChild.cpp
@@ +106,5 @@
> +
> +bool
> +SmsChild::DeallocPMobileMessageCursor(PMobileMessageCursorChild* aActor)
> +{
> +  delete aActor;

You can't do this... Your MobileMessageCursorChild is a refcounted object. You need to change this to:

  static_cast<MobileMessageCursorChild>(aActor)->Release();

And then you need to make sure that IPDL actually holds a reference. There are two ways to do that.

1. Right now you manually create an MobileMessageCursorChild object and then pass it to the SmsChild::SendPMobileMessageCursorConstructor(PMobileMessageCursorChild*) function. Since you're not doing anything really special in your create sequence you could change this to call theSmsChild::SendPMobileMessageCursorConstructor(CreateMessageCursorRequest&) function instead, and add an extra reference inside SmsChild::AllocPMobileMessageCursor. That keeps the refcount trickery localized, and it's my preferred solution here.

2. The other option is to do add an extra reference inside SmsIPCService::CreateMessageCursor and keep the SmsChild::AllocPMobileMessageCursor unmodified. That works too but spreads out the reference counting stuff and so you'd need to add some comments saying where they're eventually balanced.

You also have the same basic problem in the SmsParent/MobileMessageCursorParent code, only there you have the alloc method already doing something so you can easily do solution 1 there.

@@ +228,5 @@
> +
> +// nsICursorContinueCallback
> +
> +NS_IMETHODIMP
> +MobileMessageCursorChild::HandleContinue()

You shouldn't be able to call HandleContinue() more than once before receiving a response. Is that state managed at some higher level (like, in the DOM maybe)?

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +226,5 @@
> +
> +bool
> +SmsParent::DeallocPMobileMessageCursor(PMobileMessageCursorParent* aActor)
> +{
> +  delete aActor;

Same problem here with deleting a refcounted object.

@@ +356,5 @@
> +  return NS_SUCCEEDED(mContinueCallback->HandleContinue());
> +}
> +
> +bool
> +MobileMessageCursorParent::DoRequest(const CreateMessageCursorRequest& aRequest)

This method is a little strange, maybe you should just move this code up to RecvPMobileMessageCursorConstructor and then remove this altogether? That seems to fit better to me, but this works too, so it's up to you.

@@ +367,5 @@
> +  bool reverse = aRequest.reverse();
> +
> +  nsresult rv = dbService->CreateMessageCursor(filter, reverse, this,
> +                                               getter_AddRefs(mContinueCallback));
> +  return NS_SUCCEEDED(rv);

If this (or any of the other calls in this function) fails then the child process will be killed. Wouldn't it be better to send a NotifyError() here?

@@ +387,5 @@
> +MobileMessageCursorParent::NotifyCursorResult(nsISupports* aResult)
> +{
> +  MOZ_ASSERT(mContinueCallback);
> +
> +  SmsMessage* message = static_cast<SmsMessage*>(aResult);

This scares me too but I'll wait for the fun in bug 849739.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 58 • 12 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #56)
> PIndexedDBCursor creates PIndexedDBRequest each time Continue() is called,
> right?

Yes.

> If we follow its design in SMS/MMS database, that means we have to
> create multiple PMobileMessageCursorParent instances during one cursor
> session.

No, there's only one PMobileMessageCursorParent actor, but then multiple "continue" actors. It works out to the same number of messages and the lifetime is managed just as you want in comment 3. The big difference is that each call to Continue() is uniquely tied to one call to Notify(), something that you can't get without runtime state checking with your current design.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 59 • 12 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #54)
> > +  JSAutoRequest ar(cx);
> > +  JSAutoCompartment ac(cx, global);
> 
> I think Jonas had concern about the two lines actually. They're not called
> in MobileMessageCallback::NotifySuccess, but called in
> SmsRequest::SetSuccessInternal. Which one is more precise for these case?

Yeah, he said this:

> Do you really need to push a context since you are also pushing a request
> and a compartment here? 

So the question was actually about the context push. But I think all three are needed here because you're doing some manual JS wrapping. Pushing a context doesn't guarantee that you have a request or are in the right compartment, and XPConnect still sometimes needs the context stack in order to make security decisions. In most cases though this is all mostly a no-op I hope.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 60 • 12 years ago

(In reply to ben turner [:bent] from comment #57)
> Comment on attachment 728094 [details] [diff] [review]
> Part 3/5: IPC : v4
> 
> Review of attachment 728094 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/ipc/SmsChild.cpp
> @@ +228,5 @@
> > +
> > +// nsICursorContinueCallback
> > +
> > +NS_IMETHODIMP
> > +MobileMessageCursorChild::HandleContinue()
> 
> You shouldn't be able to call HandleContinue() more than once before
> receiving a response. Is that state managed at some higher level (like, in
> the DOM maybe)?

DOMCursor is responsible for that.

> ::: dom/mobilemessage/src/ipc/SmsParent.cpp
> @@ +356,5 @@
> > +  return NS_SUCCEEDED(mContinueCallback->HandleContinue());
> > +}
> > +
> > +bool
> > +MobileMessageCursorParent::DoRequest(const CreateMessageCursorRequest& aRequest)
> 
> This method is a little strange, maybe you should just move this code up to
> RecvPMobileMessageCursorConstructor and then remove this altogether? That
> seems to fit better to me, but this works too, so it's up to you.

In bug 849739, we'll have two cursor-based methods GetMessages/GetThreads. So RecvPMobileMessageCursorConstructor() will then contain a switch-case and one more DoRequest override will be added. See bug 849739 part 3/5 (attachment 728109 [details] [diff] [review]).

> @@ +367,5 @@
> > +  bool reverse = aRequest.reverse();
> > +
> > +  nsresult rv = dbService->CreateMessageCursor(filter, reverse, this,
> > +                                               getter_AddRefs(mContinueCallback));
> > +  return NS_SUCCEEDED(rv);
> 
> If this (or any of the other calls in this function) fails then the child
> process will be killed. Wouldn't it be better to send a NotifyError() here?

Sure. Will change related calls as well.

> @@ +387,5 @@
> > +MobileMessageCursorParent::NotifyCursorResult(nsISupports* aResult)
> > +{
> > +  MOZ_ASSERT(mContinueCallback);
> > +
> > +  SmsMessage* message = static_cast<SmsMessage*>(aResult);
> 
> This scares me too but I'll wait for the fun in bug 849739.

Yeah, hang up there :P
It's already in bug 849739 part 3/5 (attachment 728109 [details] [diff] [review]).

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 61 • 12 years ago

Replace Notify{Error,Done} with __delete__ PMobuleMessageCursor. Now whenever a cursor transaction is done, the IPC parent notifies child and removes itself from managee to ensure both sides will be no longer referenced by IPDL itself.

Carrying sr+ for DOM interface changes.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 62 • 12 years ago

Address review comment #52, fixes MobileMessageCallback as well.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 63 • 12 years ago

fix review comment 57, AddRef/Release for both child and parent. Now whenever a cursor transaction is done, the IPC parent notifies child and removes itself from managee to ensure both sides will be no longer referenced by IPDL itself.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 64 • 12 years ago

Don't move createDomMessageFromRecord() out of the service class, that's not really necessary.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 65 • 12 years ago

Comment on attachment 729480 [details] [diff] [review]
Part 1/5: interface changes : v5

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

::: dom/mobilemessage/src/ipc/PMobileMessageCursor.ipdl
@@ +12,5 @@
> +namespace mozilla {
> +namespace dom {
> +namespace mobilemessage {
> +
> +protocol PMobileMessageCursor

This won't work, you've introduced the possibility for these two simultaneous messages:

1. parent -> __delete__ -> child
2. parent <-  Continue  <- child

The child process will get killed if these messages race once the parent realizes that it has received a message addressed to a deleted actor.

The old way was better. Let's just do the cleanup in a followup bug?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 66 • 12 years ago

Comment on attachment 729482 [details] [diff] [review]
Part 3/5: IPC : v5

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

I'll need to see another version of this patch once you fix the protocol in part 1. But this looks basically right.

::: dom/mobilemessage/src/ipc/SmsChild.cpp
@@ +106,5 @@
> +
> +bool
> +SmsChild::DeallocPMobileMessageCursor(PMobileMessageCursorChild* aActor)
> +{
> +  // MobileMessageCursorChild is refcounted, must not freed manually.

Cool, also please say where the reference you're releasing comes from (SmsIPCService::CreateMessageCursor).

::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +117,5 @@
> +
> +  PMobileMessageCursorChild* child =
> +    gSmsChild->SendPMobileMessageCursorConstructor(actor,
> +      CreateMessageCursorRequest(data, aReverse));
> +  NS_ENSURE_TRUE(child, NS_ERROR_FAILURE);

If this fails you'll leak 'actor'. Best to do the extra addref right before you return NS_OK.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 67 • 12 years ago

Comment on attachment 729480 [details] [diff] [review]
Part 1/5: interface changes : v5

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

Ok, after thinking about this a little more I think we can prevent the racing destructor just fine with state machine enforcement (and, since we don't have that, with higher level logic). But this doesn't fix my objection to the previous version (calls to Continue() are still not uniquely associated with a call to Notify()) so I'd still want us to clean this up in a followup. Does that make sense?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 68 • 12 years ago

Comment on attachment 729482 [details] [diff] [review]
Part 3/5: IPC : v5

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

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +349,5 @@
> +
> +NS_IMPL_ISUPPORTS1(MobileMessageCursorParent, nsIMobileMessageCursorCallback)
> +
> +void
> +MobileMessageCursorParent::ActorDestroy(ActorDestroyReason aWhy)

You'll need to handle this in the parent. (Child is probably fine not handling this case).

Consider this scenario:

1. Child calls Continue()
2. Parent gets RecvContinue(), calls HandleContinue() (async)
3. Child crashes
4. Parent's NotifyCursorResult runs, calls SendNotifyResult()

At that point we'll try to send a message to a process that has crashed (which prints a big scary warning at least). Then I think we'll hold mContinueCallback alive forever since we'll never do anything that will trigger NotifyCursorError.

To avoid that we just need to null out mContinueCallback here and then null-check in NotifyCursorError/NotifyCursorResult/NotifyCursorDone.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 69 • 12 years ago

(In reply to ben turner [:bent] from comment #67)
> Comment on attachment 729480 [details] [diff] [review]
> Part 1/5: interface changes : v5
> 
> Review of attachment 729480 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Ok, after thinking about this a little more I think we can prevent the
> racing destructor just fine with state machine enforcement (and, since we
> don't have that, with higher level logic). But this doesn't fix my objection
> to the previous version (calls to Continue() are still not uniquely
> associated with a call to Notify()) so I'd still want us to clean this up in
> a followup. Does that make sense?

Continue/Notify pairing resides in DOMRequest/DOMCursor. There are already assertions for what you're worry about. Why can't we build a faster architecture based on these assumptions?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 70 • 12 years ago

(In reply to ben turner [:bent] from comment #68)
> Part 3/5: IPC : v5
> Review of attachment 729482 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/mobilemessage/src/ipc/SmsParent.cpp
> @@ +349,5 @@
> > +
> > +NS_IMPL_ISUPPORTS1(MobileMessageCursorParent, nsIMobileMessageCursorCallback)
> > +
> > +void
> > +MobileMessageCursorParent::ActorDestroy(ActorDestroyReason aWhy)
> 
> You'll need to handle this in the parent. (Child is probably fine not
> handling this case).
> 
> Consider this scenario:
> 
> 1. Child calls Continue()
> 2. Parent gets RecvContinue(), calls HandleContinue() (async)
> 3. Child crashes
> 4. Parent's NotifyCursorResult runs, calls SendNotifyResult()
> 
> At that point we'll try to send a message to a process that has crashed
> (which prints a big scary warning at least). Then I think we'll hold
> mContinueCallback alive forever since we'll never do anything that will
> trigger NotifyCursorError.
> 
> To avoid that we just need to null out mContinueCallback here and then
> null-check in NotifyCursorError/NotifyCursorResult/NotifyCursorDone.

Nice catch. We'd better null out mContinueCallback in ActorDestroy(), but since we returns NS_ERROR_FAILURE in SendNotifyResult(), which causes an exception in javascript caller, we should never ever have another NotifyFoo again. Besides, there are already MOZ_ASSERT(mContinueCallback) in NotifyFoo.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 71 • 12 years ago

1) Address review comments in comment 66.
2) Null out mContinueCallback in MobileMessageCursorParent::ActorDestroy() only. See comment 68 and comment 70.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 72 • 12 years ago

Refactor MobileMessageCursorParent::DoRequest() only.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 73 • 12 years ago

The last patch doesn't address review comment 57. Fixed.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 74 • 12 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #70)
> but since we returns NS_ERROR_FAILURE in SendNotifyResult()

No, I guess I didn't explain the problem well enough. The issue is that once the child process crashes all the actors get cleaned up (ActorDestroy is called, then Dealloc) and it is no longer possible to send messages using those actors. So, here's the scenario again, but maybe a little clearer:

1. Child sends Continue message to parent and then immediately crashes.
2. Parent gets RecvContinue() message, calls HandleContinue(). This is an async operation and it kicks off database activity on another thread, etc.
3. Parent receives notification that the child process crashed. It calls ActorDestroy, then Dealloc. The MobileMessageCursorParent isn't actually dead (because it's refcounted and held alive by the SMS code while we're looking up info in the database) but it's "actor parts" *are* now dead.
4. Database stuff completes and NotifyCursorResult runs. At this point it can't call SendNotifyResult() because the "actor parts" are dead.

Basically you need to set some kind of state that says "from IPDL's perspective I am dead" so that you don't call any Send* functions after that point. Luckily you also need to null out mContinueCallback to keep it from living forever. So you can use a null mContinueCallback as your state that says "from IPDL's perspective I am dead". Then you can null-check in all the Notify* functions so that you don't call Send* any more. (And of course you can't MOZ_ASSERT(mContinueCallback) any more because there are legitimate cases where it could be null.)

Does that make sense? I realize this is kind of a complicated thing to try to write about (and I doubt I'm very good at explaining it anyway) so please tell me if you need more clarification.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 75 • 12 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #69)
> Continue/Notify pairing resides in DOMRequest/DOMCursor. There are already
> assertions for what you're worry about.

Let me try an an analogy: Providing a protocol that does not enforce proper ordering is like designing a C++ class with all members and methods declared as public. Sure, you can use it correctly by adding assertions and writing documentation and having test cases. But why would you do that if you could ensure proper use by hiding members and methods?

We should design the protocol such that misuse is difficult or impossible.

> Why can't we build a faster architecture based on these assumptions?

As I pointed out earlier it boils down to the same number of actual IPC messages so performance concerns should be negligible.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 76 • 12 years ago

(In reply to ben turner [:bent] from comment #75)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #69)
> > Continue/Notify pairing resides in DOMRequest/DOMCursor. There are already
> > assertions for what you're worry about.
> 
> Let me try an an analogy: Providing a protocol that does not enforce proper
> ordering is like designing a C++ class with all members and methods declared
> as public. Sure, you can use it correctly by adding assertions and writing
> documentation and having test cases. But why would you do that if you could
> ensure proper use by hiding members and methods?
> 
> We should design the protocol such that misuse is difficult or impossible.

True. But like checking input function parameters, should we check in EVERY function you've ever created inclusive of internal utility functions? The answer would probably be NO, right? They're the same thing in my point of view. I know it's nice to add 'const' whenever possible. I know we should use reference type to deliver a clear a message that this object just can't be null. All these things.

But performance is just yet another factor that we should always take into consideration in designing program interfaces. Or, why not make nsRefPtr/nsCOMPtr thread-safe? I'm not saying you're wrong. But we can just drain more performance, save unnecessary object instanciation/refcounting/... based on the fact "DOMCursor/DOMRequest don't allow unpaired operations".

And, I know you had agreed this to be a follow-up if necessary.

> > Why can't we build a faster architecture based on these assumptions?
> 
> As I pointed out earlier it boils down to the same number of actual IPC
> messages so performance concerns should be negligible.

That 'multiple "continue" actors' is exactly I'd rather avoid if possible. Again, let's move the discussion into follow-ups.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 77 • 12 years ago

(In reply to ben turner [:bent] from comment #74)
> Does that make sense? I realize this is kind of a complicated thing to try
> to write about (and I doubt I'm very good at explaining it anyway) so please
> tell me if you need more clarification.

Very clear! Thank for these detailed clarifications.

Another thing I still have question is your comment 66. After tracing PSmsChild::SendPMobileMessageCursorConstructor(), it calls DeallocSubtree() before returning 0, and DeallocSubtree() calls DeallocPMobileMessageCursor(), which releases that extra IPDL ref.

  SmsIPCService::CreateMessageCursor(...)
  {
    nsRefPtr<MobileMessageCursorChild> actor = new MobileMessageCursorChild(...);
    // Now actor have refcount = 1

    MobileMessageCursorChild* cursorChild =
      gSmsChild->SendPMobileMessageCursorConstructor(actor, ...);
    // If success, nothing changes. If failed, calls DeallocPMobileMessageCursor(),
    // which decrease refcount to 0 and delete actor

    NS_ENSURE_TRUE(child, NS_ERROR_FAILURE);
    // When we returned from above macro, dtor of nsRefPtr tries decrease refcount of
    // actor once more and BANG!!!

    // Add an extra ref for IPDL. Will be released in
    // SmsChild::DeallocPMobileMessageCursor().
    actor->AddRef();

    actor.forget(aResult);
    return NS_OK;
  }

So we should place AddRef right before SendPMobileMessageCursorConstructor will be called:

  SmsIPCService::CreateMessageCursor(...)
  {
    nsRefPtr<MobileMessageCursorChild> actor = new MobileMessageCursorChild(...);
    // Now actor have refcount = 1

    // Add an extra ref for IPDL. Will be released in
    // SmsChild::DeallocPMobileMessageCursor().
    actor->AddRef();
    // Now actor have refcount = 2

    MobileMessageCursorChild* cursorChild =
      gSmsChild->SendPMobileMessageCursorConstructor(actor, ...);
    // If success, nothing changes. If failed, calls DeallocPMobileMessageCursor(),
    // which decrease refcount to 1

    NS_ENSURE_TRUE(child, NS_ERROR_FAILURE);
    // When we returned from above macro, dtor of nsRefPtr tries decrease refcount to
    // 0 and free the actor.

    actor.forget(aResult);
    // refcount is 1
    return NS_OK;
  }

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 78 • 12 years ago

Address comment 74 and comment 77.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 79 • 12 years ago

Comment on attachment 730688 [details] [diff] [review]
Part 3/5: IPC : v9

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

This looks great! Only some nits below.

::: dom/mobilemessage/src/ipc/SmsChild.cpp
@@ +106,5 @@
> +
> +bool
> +SmsChild::DeallocPMobileMessageCursor(PMobileMessageCursorChild* aActor)
> +{
> +  // MobileMessageCursorChild is refcounted, must not freed manually.

Nit: "must not be freed manually"

@@ +229,5 @@
> +MobileMessageCursorChild::HandleContinue()
> +{
> +  MOZ_ASSERT(mCursorCallback);
> +
> +  return SendContinue() ? NS_OK : NS_ERROR_FAILURE;

Nit: This could just be:

  SendContinue();
  return NS_OK;

In child processes any IPC failure automatically triggers process exit, so you'll never be able to return NS_ERROR_FAILURE here.

::: dom/mobilemessage/src/ipc/SmsIPCService.cpp
@@ +118,5 @@
> +
> +  PMobileMessageCursorChild* child =
> +    gSmsChild->SendPMobileMessageCursorConstructor(actor,
> +      CreateMessageCursorRequest(data, aReverse));
> +  NS_ENSURE_TRUE(child, NS_ERROR_FAILURE);

Nit: Here again it is not really possible to handle failure. Since you're in a child process if this fails the process will exit before returning.

::: dom/mobilemessage/src/ipc/SmsParent.cpp
@@ +231,5 @@
> +
> +bool
> +SmsParent::DeallocPMobileMessageCursor(PMobileMessageCursorParent* aActor)
> +{
> +  // MobileMessageCursorParent is refcounted, must not freed manually.

Nit: "must not be freed manually"

@@ +352,5 @@
> +void
> +MobileMessageCursorParent::ActorDestroy(ActorDestroyReason aWhy)
> +{
> +  // When parent fails to SendNotifyResult() in NotifyCursorResult(), it's
> +  // destroyed without nulling out mContinueCallback. So here it is.

Nit: This now serves two purposes: handling the leak and telling us if the child crashed. This comment should probably mention the second thing now.

@@ +364,5 @@
> +
> +  nsresult rv = mContinueCallback->HandleContinue();
> +  if (NS_FAILED(rv)) {
> +    rv = NotifyCursorError(nsIMobileMessageCallback::INTERNAL_ERROR);
> +  }

Nit: I think it's cleaner to write this way, but if you like this better I guess I don't mind:

  if (NS_FAILED(mContinueCallback->HandleContinue())) {
    return NotifyCursorError(nsIMobileMessageCallback::INTERNAL_ERROR);
  }
  return true;

@@ +386,5 @@
> +  }
> +
> +  if (NS_FAILED(rv)) {
> +    rv = NotifyCursorError(nsIMobileMessageCallback::INTERNAL_ERROR);
> +  }

Nit: To clean this up a little, you could do:

  if (NS_FAILED(rv)) {
    return NotifyCursorError(nsIMobileMessageCallback::INTERNAL_ERROR);
  }
  return true;

@@ +398,5 @@
> +MobileMessageCursorParent::NotifyCursorError(int32_t aError)
> +{
> +  // Consider a child dies before this asynchronous notification,
> +  // ActorDestroy() is called and mContinueCallback is set to nullptr. So we
> +  // should return an error instead of placing an assertion here.

Nit: I think it sounds a little better to say:

  The child process could die before this asynchronous notification,
  in which case ActorDestroy() was called and mContinueCallback is now
  null. Return an error here to avoid sending a message to the dead
  process.

Does that work? You could do the same in the next method too.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 80 • 12 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #77)
> So we should place AddRef right before SendPMobileMessageCursorConstructor

Yeah, you're right that the Dealloc happens inside the failed constructor call... That's kind of unfortunate.

Luckily since this happens in a child process any constructor failure just triggers a process exit so it's actually harmless in this specific case. If this were the parent process we'd need to worry about it though!

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 81 • 12 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #76)
> That 'multiple "continue" actors' is exactly I'd rather avoid if possible.
> Again, let's move the discussion into follow-ups.

Sure. Happy to.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 82 • 12 years ago

Address review comments.

	Gregor Wagner [:gwagner]
	Comment 83 • 12 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #64)
> Created attachment 729484 [details] [diff] [review]
> Part 4/5: RIL : v5
> 

I applied all patches and used your gaia changes but the number matching doesn't work for me.
1) I send a sms to the local number for example: 6501234567
2) When I reply I also see a new thread with +16501234567
3) After restarting the app I only see the 6501234567 thread but after replaying again it goes back to 2)

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 84 • 12 years ago

(In reply to Gregor Wagner [:gwagner] from comment #83)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #64)
> > Created attachment 729484 [details] [diff] [review]
> > Part 4/5: RIL : v5
> > 
> 
> I applied all patches and used your gaia changes but the number matching
> doesn't work for me.
> 1) I send a sms to the local number for example: 6501234567
> 2) When I reply I also see a new thread with +16501234567
> 3) After restarting the app I only see the 6501234567 thread but after
> replaying again it goes back to 2)

I have no idea why you have such situation. This case should have been fixed in bug 833491, which had been verified by the new test cases[1] introduced in the same bug. Besides, recently in bug 850127 and bug 850132, we're fixing problems based on this existing feature. I'll verify it again on m-c anyway.

[1]: https://github.com/mozilla/releases-mozilla-central/blob/master/dom/mobilemessage/tests/marionette/test_filter_mixed.js#L358 , https://github.com/mozilla/releases-mozilla-central/blob/master/dom/mobilemessage/tests/marionette/test_filter_mixed.js#L377

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 85 • 12 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #84)
> (In reply to Gregor Wagner [:gwagner] from comment #83)
> > I applied all patches and used your gaia changes but the number matching
> > doesn't work for me.
> > 1) I send a sms to the local number for example: 6501234567
> > 2) When I reply I also see a new thread with +16501234567
> > 3) After restarting the app I only see the 6501234567 thread but after
> > replaying again it goes back to 2)
> 
> I have no idea why you have such situation.

This is not related to DOMCursor thing here because you can still find the database stitches the two messages in the same thread from logcat. However, Gaia cannot correctly handle this situation without bug 850127 and the solution is already available in bug 851032.

	Joe Cheng [:jcheng] (please needinfo)
	Comment 86 • 12 years ago

leo+ as this is a part of MMS. No_UPLIFT for now before the whole MMS is completed

	Gregor Wagner [:gwagner]
	Comment 87 • 12 years ago

Comment on attachment 729484 [details] [diff] [review]
Part 4/5: RIL : v5

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

So in general I think we shouldn't uplift this patch. The actual code that deals with MMS is tiny in comparison to the size of this patch and we completely put the SMS feature at risk.

::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
@@ +1380,5 @@
> +      let cursor = event.target.result;
> +      // Once the cursor has retrieved all keys that matches its key range,
> +      // the filter search is done.
> +      if (cursor) {
> +        let timestamp = Array.isArray(cursor.key) ? cursor.key[1] : cursor.key;

When can this be an array?

@@ +1381,5 @@
> +      // Once the cursor has retrieved all keys that matches its key range,
> +      // the filter search is done.
> +      if (cursor) {
> +        let timestamp = Array.isArray(cursor.key) ? cursor.key[1] : cursor.key;
> +        if (collect(txn, cursor.primaryKey, timestamp)) {

I don't like that we use collect with variable number of arguments. That seems error prone.

@@ +1385,5 @@
> +        if (collect(txn, cursor.primaryKey, timestamp)) {
> +          cursor.continue();
> +        }
> +      } else {
> +        collect(txn, 0, 0);

Can we use some constants here so that we know what the arguments mean?

@@ +1481,5 @@
> +    if (filter.numbers) {
> +      if (DEBUG) debug("filter.numbers " + filter.numbers.join(", "));
> +
> +      if (!single) {
> +        collect = intersectionCollector.newContext();

I don't like this context stuff. That seems to be too complicated.

@@ +1764,5 @@
> +      return false;
> +    }
> +
> +    let tres = contexts[0].results;
> +    let qres = contexts[1].results;

I know this is already in the current code and I said I don't like this approach but we should at least use constants here so we know what context 0 and 1 is.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 88 • 12 years ago

(In reply to Gregor Wagner [:gwagner] from comment #87)
> Part 4/5: RIL : v5
> Review of attachment 729484 [details] [diff] [review]:
> -----------------------------------------------------------------

Patches needed are needed. That doesn't change because of a NO_UPLIFT flag. Things happened in this patch is for bug 849739 and bug 854790, which are both leo+. In bug 849739 we rewrite getThreadList() with the same DOMCursor based technique, and reuse the DOM/IPC code we have implemented in this bug. In bug 854790, we reuse refactored database code in this patch. I made all these because we need them, not I want them.

And about the risks, EVERY SMS filter condition has its own test case. And EVERY combination of these conditions has its own test case, too. Test cases are always not sufficient enough, but sometimes they do mean more than nothing.

> ::: dom/mobilemessage/src/ril/MobileMessageDatabaseService.js
> @@ +1380,5 @@
> > +      let cursor = event.target.result;
> > +      // Once the cursor has retrieved all keys that matches its key range,
> > +      // the filter search is done.
> > +      if (cursor) {
> > +        let timestamp = Array.isArray(cursor.key) ? cursor.key[1] : cursor.key;
> 
> When can this be an array?

All indexes in message store are multi-key, except 'timestamp'. We returns filtered results in timestamp order. So when you're filtering with 'read', you have to compare both 'read' and 'timestamp'. In this case, the key value is |[<read>, <timestamp>]|. But when you're simply want all messages ordered by timestamp, the key value is <timestamp> only. That was done in bug 813978.

> @@ +1381,5 @@
> > +      // Once the cursor has retrieved all keys that matches its key range,
> > +      // the filter search is done.
> > +      if (cursor) {
> > +        let timestamp = Array.isArray(cursor.key) ? cursor.key[1] : cursor.key;
> > +        if (collect(txn, cursor.primaryKey, timestamp)) {
> 
> I don't like that we use collect with variable number of arguments. That
> seems error prone.

Actually it takes three, and the last one is optional. I'll make it more clear.

> @@ +1481,5 @@
> > +    if (filter.numbers) {
> > +      if (DEBUG) debug("filter.numbers " + filter.numbers.join(", "));
> > +
> > +      if (!single) {
> > +        collect = intersectionCollector.newContext();
> 
> I don't like this context stuff. That seems to be too complicated.

It's complicated because the requirement is complicated. File a bug and remove SmsFilter completely and then we can get rid of these complicated things.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 89 • 12 years ago

Rebase

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 90 • 12 years ago

Rebase

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 91 • 12 years ago

Address review comment 87, add comments and use pre-defined constants.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 92 • 12 years ago

Rebase

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 93 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc3d0f45fd4a
https://hg.mozilla.org/integration/mozilla-inbound/rev/148460765688
https://hg.mozilla.org/integration/mozilla-inbound/rev/705364b9c45c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5664d2d50c0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f43fddf08e85

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 94 • 12 years ago

Update landed patch. Fix android 2.2 debug build.

	Ed Morley [:emorley]
	Comment 95 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/cc3d0f45fd4a
https://hg.mozilla.org/mozilla-central/rev/148460765688
https://hg.mozilla.org/mozilla-central/rev/705364b9c45c
https://hg.mozilla.org/mozilla-central/rev/5664d2d50c0a
https://hg.mozilla.org/mozilla-central/rev/f43fddf08e85

	Ryan VanderMeulen [:RyanVM]
	Comment 96 • 12 years ago

Unfortunately, I had to back this push out because the new test for bug 849739 was failing frequently. Not knowing the how these depended on each other, I backed out the entire push. Sorry for the hassle :(.
https://hg.mozilla.org/integration/mozilla-inbound/rev/81dc5a203ac2

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 97 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f45aa3bf28cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac2be78cb5e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/11aa80c27732
https://hg.mozilla.org/integration/mozilla-inbound/rev/94b4e7b38533
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ff31e921833

	Ryan VanderMeulen [:RyanVM]
	Comment 98 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/f45aa3bf28cb
https://hg.mozilla.org/mozilla-central/rev/ac2be78cb5e7
https://hg.mozilla.org/mozilla-central/rev/11aa80c27732
https://hg.mozilla.org/mozilla-central/rev/94b4e7b38533
https://hg.mozilla.org/mozilla-central/rev/0ff31e921833

	Julien Wajsberg [:julienw]
	Comment 99 • 12 years ago

is commercial RIL 1.1 ready for this ?

	Julien Wajsberg [:julienw]
	Comment 100 • 12 years ago

Just read my mails ;) and it seems we can indeed uplift. So let's go.

	Ryan VanderMeulen [:RyanVM]
	Comment 101 • 12 years ago

Leaving these to Vicamo for the uplift.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 102 • 12 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #101)
> Leaving these to Vicamo for the uplift.

I'm leaving for Madrid in a few hours. Will probably land all these until then.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 103 • 12 years ago

Uplift WIP: https://github.com/vicamo/b2g_mozilla-central/tree/bugzilla/838467/b2g18 , verifying.

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 104 • 12 years ago

dom/mobilemessage/src/MobileMessageCursorCallback.cpp: In static member function 'static nsresult mozilla::dom::mobilemessage::MobileMessageCursorCallback::cycleCollection::TraverseImpl(mozilla::dom::mobilemessage::MobileMessageCursorCallback::cycleCollection*, void*, nsCycleCollectionTraversalCallback&)':
dom/mobilemessage/src/MobileMessageCursorCallback.cpp:19: error: 'nsISupports' is an ambiguous base of 'mozilla::dom::DOMCursor'

Seems depending on bug 824986

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 105 • 12 years ago

On last problem, hopefully. nsTArray interface changes in m-c, brings more utility functions, casting support. All are unavailable in b2g18 branch. We get a 'no matching function for call to nsTArrayToJSArray' error.

	Julien Wajsberg [:julienw]
	Comment 106 • 12 years ago

Do you think you'll be able to uplift this soon ?

	Vicamo Yang [:vicamo][:vyang] Assignee
	Comment 107 • 12 years ago

(In reply to Julien Wajsberg [:julienw] from comment #106)
> Do you think you'll be able to uplift this soon ?

Local build passed, now trying builds on other platforms (because I got backed-out once for failed to build fennec with b2g18 branch).