System messages handler is fired twice if there was already an handler for b.html contained into an iframe a.html and the page navigate to b.html

RESOLVED FIXED in Firefox 21

Status

Firefox OS
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: salva, Assigned: fabrice)

Tracking

unspecified
B2G C4 (2jan on)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

How to reproduce:

Flash Gaia with Cost Control's DEBUG flag set to true (apps/costcontrol/js/utils/debug.js)

 1- Open Cost Control application
 2- Press home button to send the app to background
 3- Open dialer
 4- Enter any number
 5- Press call button
 6- Immediately, press end call button
 7- Check logcat

Expected:
 - One message saying: "Outgoing call finished!"

Actual:
 - Two messages instead of one
Summary: System messages are received even when the location has been changed → System messages handler is fired twice if there was already an handler for b.html contained into an iframe a.html and the page navigate to b.html
Blocks: 755245
What seems to happen is:

Cost Control application is first opened with the url a.html. This a.html page contains an iframe with src="b.html". b.html calls navigator.mozSetMessageHandler('telephony-call-ended', callback);

Then Cost control application goes to the background.

When a call is finished the platform send one system message 'telephony-call-ended' wich triggers a open-app message in Gaia targeting the cost control application. The window manager in Gaia change the src attribute of the iframe to b.html (this is the target registered to handle such system message).

When the frame src change it loads b.html that call again navigator.mozSetMessageHandler('telephony-call-ended', callback).

Funilly the callback is called twice instead of once. I strongly suspect that this line http://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageInternal.js#98 is not the right check to do since it filters only on the url while the content window has changed but not the url.
This sounds like a platform problem. Possibly related to bfcache.
Assignee: 21 → mrbkap
blocking-basecamp: ? → +
SystemMessageInternal.broadcastMessage() is called once with:

> telephony-call-ended {"number":"<number>","duration":0,"direction":"outgoing"}

SystemMessageInternal._sendMessageCommon() is called twice:

> [10, telephony-call-ended] --> {"number":"<number>","duration":0,"direction":"outgoing"}
> [8, telephony-call-ended] --> {"number":"<number>","duration":0,"direction":"outgoing"}

So there are two messages, one with the windowID 10 and one with the windowID 8. Not sure what's going on. These two messages are received by *one* handler in message_handler.js:

> ++ received(0.6944369836071996) --> {"number":"<number>","duration":0,"direction":"outgoing"}
> ++ received(0.6944369836071996) --> {"number":"<number>","duration":0,"direction":"outgoing"}

The number at the beginning is just a variable with a random number I enclosed in the message handler to see if we have multiple ones.
It's the same SystemMessageManager instance that receives both of these messages - I checked SMM._id when 'telephony-call-ended' is received.
For windowIDs 8 and 10 there are these registered SystemMessageManagers:

> SystemMessageManager:Register[{675cf5ab-8af1-4496-b682-33e5984df9a4}, 10]
> SystemMessageManager:Register[{6e32dd10-3833-4eb0-9f2a-0bb91e2a6d4d}, 8]

Messages only arrive at the first one, seems like it handles messages for windowID=8 as well.
This is totally a bug in the system message stuff... No bfcache interaction at all.
SystemMessageInternal._listeners[manifest] contains the same target twice. Looks like aMessage.target is the same when receiving 'SystemMessageManager:Register' from windowID 8 and 10 here:

http://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageInternal.js#195
Here's the bug: the system message stuff registers messages in the parent process by first manifest and then inner window ID. Based on that pair, it registers a frame message manager. This works fine as long as there is not ever any navigation. If the frame in question is closed, then we'll unregister that frame message manager and most of our "navigation" in apps is simply hash changes (and therefore doesn't actually navigate). In this case, we actually do have navigation, meaning that we register the same frame message manager twice and start sending duplicated messages. I'm working on a patch.
Created attachment 701054 [details] [diff] [review]
patch -w

Here's a diff -w of the fix (since I did a tiny bit of whitespace cleanup which makes the original patch harder to read). I'll attach a full diff in a second with more of an explanation.
Attachment #701054 - Flags: review?(fabrice)
(Assignee)

Comment 10

5 years ago
ccing Gene that did most of the last changes in this part of the codebase.
Comment on attachment 701054 [details] [diff] [review]
patch -w

Fabrice pointed out that this doesn't cover the same origin case.
Attachment #701054 - Attachment is obsolete: true
Attachment #701054 - Flags: review?(fabrice)
Component: General → Gaia
Target Milestone: --- → B2G C4 (2jan on)
Component: Gaia → General
Created attachment 701132 [details] [diff] [review]
patch v2 beta

There's a change that doesn't belong here (and I need to clean up my terminology) but this seems to work.
There was a history why we need to separately save the targets by the index of window ID (bug 802564). If my recall is right, we were hoping to treat the window opened by the app itself and the window in-line-ly opened by other apps as different targets. For example, the contact-entry-adding page can be opened by the Contact app itself and can also be opened as an in-line window by the SMS app.

The patch provided here seems to assume the 2 different windows have the same target (they are coming from the same message_handler.html page) and attempt to use the target as an index to construct the listeners map to avoid broadcasting system message to the same target twice. As mentioned above, if the windows opened in-line-ly and non-inline-ly are actually different targets, then this patch looks OK. If not, it might cause regression.

Benjamin, please correct me if I'm wrong. Thanks!
Over to fabrice to complete this as Blake is traveling today and tomorrow.
Assignee: mrbkap → fabrice
(Assignee)

Comment 15

5 years ago
Comment on attachment 701132 [details] [diff] [review]
patch v2 beta

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

::: dom/messages/SystemMessageInternal.js
@@ +68,5 @@
>  
>    Services.obs.notifyObservers(this, "system-message-internal-ready", null);
>  }
>  
> +function findTarget(listeners, target) {

Nit: aListeners, aTarget
Attachment #701132 - Flags: review+
(Assignee)

Comment 16

5 years ago
That also looks good to me and I saw no failures in my tests. However, I'd like to have Benjamin double check with Gene.
(Assignee)

Comment 17

5 years ago
Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd589cb673b6
https://hg.mozilla.org/releases/mozilla-b2g18/rev/1c9fb336a40a

Feel free to open followups if needed.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-b2g18: --- → fixed
status-firefox19: --- → wontfix
status-firefox20: --- → wontfix
status-firefox21: --- → fixed
(In reply to Gene Lian [:gene] from comment #13)
> The patch provided here seems to assume the 2 different windows have the
> same target (they are coming from the same message_handler.html page) and
> attempt to use the target as an index to construct the listeners map to
> avoid broadcasting system message to the same target twice.

As far as I can tell (I haven't tested, though) this patch should handle this fine. We don't fire any message manager messages directly to specific SystemMessageManagers as none of the information in the messages is more specific than the "page" involved. The patch allows a manifest to have multiple targets and then fires a single message per target. Once we fire a message to a given frame message manager, all participating *system* message managers will have a shot at it. There's no need to send those messages twice (unless we want to start including per-window data such as a DOM request ID).
(In reply to Fabrice Desré [:fabrice] from comment #16)
> That also looks good to me and I saw no failures in my tests. However, I'd
> like to have Benjamin double check with Gene.

This patch works great to me. 
My test case:
1. open contact app
2. open sms -> open inline contact app
3. close inline contact or press home key
4. close contact app
The |winCount| works correctly.
Comment on attachment 701132 [details] [diff] [review]
patch v2 beta

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

Oops... I think there are some buggy logic in this patch. Please correct me if I'm wrong.

::: dom/messages/SystemMessageInternal.js
@@ +38,5 @@
>                    "SystemMessageManager:AskReadyToRegister",
>                    "child-process-shutdown"]
>  
>  function debug(aMsg) {
> +  dump("-- SystemMessageInternal " + Date.now() + " : " + aMsg + "\n");

We shouldn't enable this.

@@ +76,5 @@
> +      return listener;
> +  }
> +
> +  return null;
> +}

I'd prefer moving this into SystemMessageInternal as a private function but it's not a big matter.

@@ +226,5 @@
>          debug("Got child-process-shutdown from " + aMessage.target);
>          for (let manifest in this._listeners) {
> +          // See if any processes in this manifest have this target.
> +          let targets = this._listeners[manifest];
> +          for (let target = 0; target < targets.length; ++target) {

I'd prefer s/target/i or something like that.

@@ +237,5 @@
> +              } else {
> +                // There are other targets for this manifest, get rid of this
> +                // one.
> +                targets.splice(target, 1);
> +              }

Don't we need to add a break here to leave the targets loop? since the targets array have been modified (even removed...):

  if (targets[target].target === aMessage.target) {
    if (targets.length === 1) {
      ...
    } else {
      ...
    }
    break;
  }

@@ +258,5 @@
> +                // More than one left, remove this one and leave the rest.
> +                targets.splice(i, 1);
> +              }
> +
> +            }

The same. I think we should add a break here.

@@ +439,5 @@
>        }
>  
>        let winTargets = this._listeners[aManifestURI];
>        if (winTargets) {
> +        for (let target = 0; target < winTargets.length; ++target) {

s/target/i looks more comfortable to distinguish the index and the real target.
I'm working on a follow-up patch. ;)
(In reply to Gene Lian [:gene] from comment #20)
> Oops... I think there are some buggy logic in this patch. Please correct me
> if I'm wrong.

That's why I called this patch "beta" :-)

> Don't we need to add a break here to leave the targets loop? since the
> targets array have been modified (even removed...):

This would definitely be cleaner.

> The same. I think we should add a break here.

Bonus points for refactoring the two instances of the loop (which are almost the same with the winCount check)!
s/with/with the exception of/
Depends on: 830616
Comment on attachment 701132 [details] [diff] [review]
patch v2 beta

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

::: dom/messages/SystemMessageInternal.js
@@ +212,5 @@
> +        } else if (!(target = findTarget(targets, aMessage.target))) {
> +          targets[msg.manifest].push({
> +            target: aMessage.target,
> +            winCount: 1
> +          });

Wait! This looks really weird. Why not:

  targets.push({
    target: aMessage.target,
    winCount: 1
  });

This line shocks me why this patch is working...
(In reply to Gene Lian [:gene] from comment #24)
> Comment on attachment 701132 [details] [diff] [review]
> patch v2 beta
> 
> Review of attachment 701132 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/messages/SystemMessageInternal.js
> @@ +212,5 @@
> > +        } else if (!(target = findTarget(targets, aMessage.target))) {
> > +          targets[msg.manifest].push({
> > +            target: aMessage.target,
> > +            winCount: 1
> > +          });
> 
> Wait! This looks really weird. Why not:
> 
>   targets.push({
>     target: aMessage.target,
>     winCount: 1
>   });
> 
> This line shocks me why this patch is working...

After testing, this wrong logic has no change to be proceeded so far. However, we still need to fix it to allow child process to add different targets. I've already fixed up all the issues at bug 830616.
https://hg.mozilla.org/mozilla-central/rev/bd589cb673b6
You need to log in before you can comment on or make changes to this bug.