906164 - mozHasPendingMessage() should queue messages even when app is running

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

mozHasPendingMessage() only checks for pending messages in the SystemMessageInternal queue. But when a SystemMessageManager is present for a page, but it has no handler set for a particular message, it seems sensible to queue up messages and say they are queued. For example:

function A() {
  mozSetMessageHandler('alarm', function() {
    B()
  });
  navigator.mozAlarms.add(new Date(), "honorTimezone", {});
}

function B() {
  // unset the handler
  mozSetMessageHandler('alarm', null);
  // wait for alarm to go off
  setTimeout(function() {
     console.log(mozHasPendingMessage('alarm'));
  }, 1000);
  navigator.mozAlarms.add(new Date(), "honorTimezone", {});
}

It should print true, but it won't.

The thing is SystemMessageManager will receive a message, find no handler and throw the message away, when the page might want to set a handler on the basis of message availability.

Gene: Do you think this is valid. How would this additional queuing affect the delivery guarantees and especially do we need any extra wakelock behaviour because of this?

Bug 802876 reported similar behaviour, and was closed as WORKSFORME based on (what seems to me) insufficient comments in the bug about exactly what was going wrong. I believe this is what that bug originally intended to state.

Comment 1

[Gene Lian (I already quit Mozilla)]

Nice catch! Nikhil!

This is indeed a bug. If there exists any system message that has not yet been handled by the handler, we should queue it and the mozHasPendingMessage() must return true.

Regarding the CPU wake lock mechanism, I think it is still working well because the manager will return "SMM:HandleMessagesDone" in any way to release the CPU wake lock even if the handler is not set to handle the system message.

The real issue is "SMM:Message:Return:OK", we shouldn't return it to the central to cancel the queued message when the handler is not set. If so, I feel "SMM:Message:Return:OK" is working very similar to the meaning of "SMM:HandleMessagesDone" and we could even have a chance to combine them into one message.

To do so, maybe we can add a flag like "isHandlerSet": true/false in the JSON object of the combined message. The central can only cancel the queuing system message when "isHandlerSet" is true, but will release the CPU wake lock no matter "isHandlerSet" is true or false.

Comment 2

[Gene Lian (I already quit Mozilla)]

This is a wrong API behaviour. In theory, we should fix it soon. It seems mozHasPendingMessage() has not yet been widely or correctly used in the apps by far.

Comment 3

[Edgar Chen [:edgar]]

I met this wrong API behavior in bug 975950 as well.

Comment 5

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #3)
> I met this wrong API behavior in bug 975950 as well.

We need to correct the API behavior for bug 975950 which is a DSDS bug. So mark as 1.4?.

Comment 6

[Preeti Raghunath(:Preeti)]

Andrew,

Can this be moved to DSDS?

Comment 7

[Edgar Chen [:edgar]]

Attachment: WIP, patch, v1

Just a quick patch to fix this issue, didn't combine the "SMM:Message:Return:OK" and "SMM:HandleMessagesDone" together.

Comment 8

[Henry Chang [:hchang]]

Attachment: Test case for gaia-ui-test

Without applying the attached patch, this gaia-ui-test won't pass.

Comment 9

[Andrew Overholt [:overholt]]

(In reply to Preeti Raghunath(:Preeti) from comment #6)
> Andrew,
> 
> Can this be moved to DSDS?

No, I don't think so.

Comment 10

[Henry Chang [:hchang]]

Attachment: Bug906164.patch

Comment 11

[Henry Chang [:hchang]]

Comment on attachment 8384434 [details] [diff] [review]
Bug906164.patch

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

The idea is to ack the received system message right before handling it. If the DOM side SystemMessageManager is created but the handler is not set, the message will be kept in the queue in SystemMessageInternal until we "GetPendingMessage".

https://bugzilla.mozilla.org/attachment.cgi?id=8382916 is a gaia-ui-test test case. The test won't pass unless we apply this patch.

Comment 12

[Jason Smith [:jsmith]]

(In reply to Edgar Chen [:edgar][:echen] from comment #5)
> (In reply to Edgar Chen [:edgar][:echen] from comment #3)
> > I met this wrong API behavior in bug 975950 as well.
> 
> We need to correct the API behavior for bug 975950 which is a DSDS bug. So
> mark as 1.4?.

The bug cited here is not a blocker, which means this isn't going to block either.

Comment 13

[Gene Lian (I already quit Mozilla)]

Comment on attachment 8384434 [details] [diff] [review]
Bug906164.patch

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

::: dom/messages/SystemMessageManager.js
@@ +213,5 @@
>      let dispatcher = this._dispatchers[msg.type];
>      if (dispatcher) {
> +      if (aMessage.name == "SystemMessageManager:Message") {
> +        // Send an acknowledgement to parent to clean up the pending message
> +        // after we dispatched the message to app, so a re-launched app won't

a/after we dispatched/before dispatching/

Comment 15

[Henry Chang [:hchang]]

Attachment: Bug906164 for checkin (Rebased, fixed comment, r+'d)

Comment 16

[Henry Chang [:hchang]]

try server result for rebased version: 
https://tbpl.mozilla.org/?tree=Try&rev=aa1abe7ba75e

Comment 17

[sam.hua]

Hi echen,

Should it be merged into v1.3t for bug935802?

Thanks!

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Edgar Chen [:edgar]]

(In reply to sam.hua from comment #17)
> Hi echen,
> 
> Should it be merged into v1.3t for bug935802?
> 
> Thanks!

Do we still meet the issue in v1.3t? (I mean Bug 935802)
If yes, please nominate Bug 935802 for 1.3t.
Let PM to triage it.
Thank you.

Comment 21

[Jason Smith [:jsmith]]

So this patch apparently broke the dialer entirely - no notifications of incoming calls & dialing a call is no longer working.

Henry & Gene - Can you guys confirm this & backout asap if that's the case? Trunk testing of the dialer is entirely blocked here right now due to this.

Comment 22

[Henry Chang [:hchang]]

(In reply to Jason Smith [:jsmith] from comment #21)
> So this patch apparently broke the dialer entirely - no notifications of
> incoming calls & dialing a call is no longer working.
> 
> Henry & Gene - Can you guys confirm this & backout asap if that's the case?
> Trunk testing of the dialer is entirely blocked here right now due to this.

I found the root cause and will attach the patch in bug 993732.

Comment 23

[sam.hua]

v1.3t also need it

Comment 24

[Gene Lian (I already quit Mozilla)]

Be careful the patch here is actually wrong. Needs bug 993732 as the follow-up.

Comment 25

[Gene Lian (I already quit Mozilla)]

If this one is marked as v1.3t, bug 993732 should be too.

Comment 26

[Henry Chang [:hchang]]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #25)
> If this one is marked as v1.3t, bug 993732 should be too.

Actually the wrong patch will perfectly suit 1.3T because the renaming was not applied to it:

http://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/file/9ef12c19ddc9/dom/messages/SystemMessageManager.js#l209

Comment 27

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #26)
> (In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #25)
> > If this one is marked as v1.3t, bug 993732 should be too.
> 
> Actually the wrong patch will perfectly suit 1.3T because the renaming was
> not applied to it:
> 
> http://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/file/9ef12c19ddc9/dom/
> messages/SystemMessageManager.js#l209

But anyway, the patch may conflict to 1.3T so I will provide another patch for 1.3T.
Thanks.

Comment 28

[Henry Chang [:hchang]]

Attachment: Bug 906164 for 1.3T

Comment 29

[Gene Lian (I already quit Mozilla)]

(In reply to Henry Chang [:henry] from comment #26)
> Actually the wrong patch will perfectly suit 1.3T because the renaming was
> not applied to it:
> 
> http://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/file/9ef12c19ddc9/dom/
> messages/SystemMessageManager.js#l209

Oh, yes. You're right. :)

Comment 30

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

Henry's working on bug 995834, lets track bug 995834

Comment 31

[Ken Chang[:kenkai]]

(In reply to Joe Cheng [:jcheng] from comment #30)
> Henry's working on bug 995834, lets track bug 995834
I think we should do a uplift to 1.3T for this bug, and the patch of this bug is not a solution of bug 995834.
Set it as 1.3T+ and add checkin-needed flag.

Comment 32

[[:fabrice] Fabrice Desré]

 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/32bb713e6d0d

Comment 33

[Jason Smith [:jsmith]]

(In reply to Fabrice Desré [:fabrice] from comment #32)
>  https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/32bb713e6d0d

Uh oh. We needed to uplift bug 993732 with this patch to ensure that we don't cause that large wave of smoketest regressions again.

Can you get bug 993732 uplifted with this?

Comment 34

[Gene Lian (I already quit Mozilla)]

(In reply to Jason Smith [:jsmith] from comment #33)
> (In reply to Fabrice Desré [:fabrice] from comment #32)
> >  https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/32bb713e6d0d
> 
> Uh oh. We needed to uplift bug 993732 with this patch to ensure that we
> don't cause that large wave of smoketest regressions again.
> 
> Can you get bug 993732 uplifted with this?

No need. The patch that Fabrice uplifted has fixed that issue as well. ;)