802564 - Can't set window.location in inline disposition web activity when its App frame is opened

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P3)

Description

[Evelyn Hung]

I get this JavaScript Error: 
E/GeckoConsole(  106): [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "window.location is null" {file: "app://settings.gaiamobile.org/js/simcard_dialog.js" line: 300}]' when calling method: [nsIDOMSystemMessageCallback::handleMessage]" {file: "jar:file:///system/b2g/omni.ja!/components/SystemMessageManager.js" line: 84}]

STR: (WARNING: the following steps will lock your SIM card, so you need to know PUK code for unlocking)
1. open Settings app > SIM Security
2. enable/disable SIM PIN, a 'Enter SIM PIN' dialog appears.
3. keep typing wrong SIM PIN for three times to make your SIM Card locked.
4. an inline disposition web activity trigger from System app (which monitor card states), ask Settings app to pop up an 'Enter PUK code' dialog.
5. however, the dialog is empty because of the JavaScript error above.

To make the dialog become normal:
6. press home botton to dismiss the empty dialog.
7. close Settings app in Card view.
8. open Dialler app(or other apps need SIM card), the dialog will be trigger again by System app via web activity.
9. the dialog 'Enter PUK code' shows, and no JavaScript error like above one.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't really know what it means to "share the same view of an opened app".

Why are we getting window.location? Can this be worked around on the front-end side?

Comment 2

[Evelyn Hung]

It happens when both Settings App frame and inline activity frame are opened, the activity frame can't set window.location.hash to a proper div (above error occurs). If I close the App frame, the activity frame can correctly locate to the div.

Comment 3

[Andrew Overholt [:overholt]]

Is there a workaround we can use?  What activity do we need this for?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Are you sure this isn't a window object of a closed window or some such? window.location is never null (barring any very strange bugs of course) unless the window has been closed but someone is still holding a reference to it.

Comment 5

[Evelyn Hung]

Thanks for the hint. 
I was waiting for https://bugzilla.mozilla.org/show_bug.cgi?id=802566 fixed, and I suspected they are related. I will update here if I find the root case.

Comment 6

[Andrew Overholt [:overholt]]

If you determine this is a bug, Evelyn, please clear the needinfo flag and we'll discuss it again in triage.  Thanks.

Comment 7

[Evelyn Hung]

Okay, I can describe the bug more specifically.
The problem only happens on if the app was already opened, its "second time" inline web activity will fail to get window.location. I suspect it's because in the "second time" web activity, the mozSetMessageHandler callback will be invoked twice (or more). 
Kill the app from card view and then reopen it will make the web activity shows normal at the first time, and could reproduce the issue by the second web activity attempt.

Another STR in bug 805734. the same symptom.

Comment 9

[John Shih]

There is the STR on this bug and hope it's more clear to reproduce
1. Go to Settings app
2. Enable the SIM PIN in SIM Security
3. Reboot the phone
4. Unlock the phone in lockscreen
5. Press "X" on Enter SIM PIN page (then you will go to homescreen)
6. Launch the dialer

Then you can see the Enter SIM PIN page prompt up with no content

Screen shot: http://i.imgur.com/QG6dg.png

Note: it cause by the settings app is running in the meanwhile
      and the Enter SIM PIN is be launched two times
      window.location will failed in the second time as evelyn said above

Comment 10

[John Shih]

Attachment: Screenshot

Comment 11

[Andrew Overholt [:overholt]]

Gene, can you take a look and see if there's a workaround?  If not, please un-assign yourself unless you want to take this :)

Comment 12

[Gene Lian (I already quit Mozilla)]

Hi Benjamin, thanks for volunteering taking this! I'm glad to discuss with you about activities/system-messages mechanisms at any time. ;)

Comment 13

[Benjamin Chen [:bechen]]

Attachment: fix system message leak

SystemMessageInternal:
STR:
1. open sms app
2. long press home key -> remove sms app
repeat step1&2
The var _listeners grows infinitely.

SystemMessageManager:
Due to override the "observe method" in DOMRequestIpcHelper. There is no way to remove cpmm message listeners which registered by "this.initHelper"

Comment 14

[Benjamin Chen [:bechen]]

Attachment: fix system message leak

check aTopic in DOMRequestIpcHelper observe method

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 677275 [details] [diff] [review]
fix system message leak

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

Thanks Benjamin! Can you check if that fixes also bug 806959? That looks very similar to me.

Comment 16

[Gene Lian (I already quit Mozilla)]

Comment on attachment 677275 [details] [diff] [review]
fix system message leak

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

Just providing some drive-by feedback. You still need to ask Fabrice for a formal review. ;)

::: dom/messages/SystemMessageInternal.js
@@ +175,5 @@
>          let manifest = msg.manifest;
>          debug("Got Register from " + manifest);
>          if (!this._listeners[manifest]) {
>            this._listeners[manifest] = [];
>          }

I think you can move the following codes to here by adding an else block.

@@ +176,5 @@
>          debug("Got Register from " + manifest);
>          if (!this._listeners[manifest]) {
>            this._listeners[manifest] = [];
>          }
> +        

Nit: please remove the blanks.

@@ +177,5 @@
>          if (!this._listeners[manifest]) {
>            this._listeners[manifest] = [];
>          }
> +        
> +        let mm = aMessage.target;

Nit: how about s/mm/target? |mm| means messageManager in general.

@@ +178,5 @@
>            this._listeners[manifest] = [];
>          }
> +        
> +        let mm = aMessage.target;
> +        for (let manifestT in this._listeners) {

I think we can just check the targets under the this._listeners[manifest].

@@ +180,5 @@
> +        
> +        let mm = aMessage.target;
> +        for (let manifestT in this._listeners) {
> +          let index = this._listeners[manifestT].indexOf(mm);
> +          if (index != -1) { 

Nit: please remove the blanks.

@@ +181,5 @@
> +        let mm = aMessage.target;
> +        for (let manifestT in this._listeners) {
> +          let index = this._listeners[manifestT].indexOf(mm);
> +          if (index != -1) { 
> +            debug("already in _listeners" + index);

debug("Target is already in _listeners[manifest] at index: " + index);

::: dom/messages/SystemMessageManager.js
@@ +193,5 @@
>    observe: function sysMessMgr_observe(aSubject, aTopic, aData) {
>      if (aTopic === kSystemMessageInternalReady) {
>        this._registerManifest();
>      }
> +    DOMRequestIpcHelper.prototype.observe.call(this, aSubject, aTopic, aData);

Nice work!

Damn! this is my fault that I overrode the observe()! Sorry...

Comment 17

[Benjamin Chen [:bechen]]

Attachment: fix system message leak

Hi Fabrice:
This patch should be able to fix bug 806959 too. The same root cause.
Please help to review this patch.
Thanks a lot

Comment 18

[[:fabrice] Fabrice Desré]

Comment on attachment 677323 [details] [diff] [review]
fix system message leak

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

r- because that doesn't really fix the problem from bug 806959. Here you prevent multiple mm to be added, but we never remove then when navigating away from a page that sets them.

So if page1.html is loaded and calls mozSetMessageHandler(), if you navigate to page2.html you must remove the message handler. This is the approach in the wip I had in bug 806959, and that didn't work because of the observer issue that you fixed here. My opinion is that we probably need both approaches combined.

::: dom/messages/SystemMessageInternal.js
@@ +176,5 @@
>          debug("Got Register from " + manifest);
>          if (!this._listeners[manifest]) {
>            this._listeners[manifest] = [];
>          }
> +        

Nit: remove the trailing whitespace.

@@ +177,5 @@
>          if (!this._listeners[manifest]) {
>            this._listeners[manifest] = [];
>          }
> +        
> +        if (this._listeners[manifest].indexOf(aMessage.target) != -1) { 

same here

@@ +181,5 @@
> +        if (this._listeners[manifest].indexOf(aMessage.target) != -1) { 
> +          debug("Target is already in _listeners[manifest]");
> +          return;
> +        }
> +        

same here.

Comment 19

[Benjamin Chen [:bechen]]

Attachment: fix system message leak

Hi Fabrice:
In this patch, I merged your solution in bug 806959 for unregistered system message. And also added "innerWindowID" to distinguish the app is inline or non-inline so that we can handle the message listeneres in _listeners correctly.

Comment 20

[[:fabrice] Fabrice Desré]

Comment on attachment 678199 [details] [diff] [review]
fix system message leak

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

r=me with comments addressed

::: dom/messages/SystemMessageInternal.js
@@ +82,5 @@
>      debug("Sending " + aType + " " + JSON.stringify(aMessage) +
>        " for " + aPageURI.spec + " @ " + aManifestURI.spec);
>      if (this._listeners[aManifestURI.spec]) {
> +      for (let winID in this._listeners[aManifestURI.spec]) {
> +        if (this._listeners[aManifestURI.spec][winID]) {

Do we really need this test? Can ever this._listeners[aManifestURI.spec][winID] be null/undefined?

Also, use a local variable to hold this._listeners[aManifestURI.spec]

@@ +134,5 @@
>      this._pages.forEach(function(aPage) {
>        if (aPage.type == aType) {
>          if (this._listeners[aPage.manifest]) {
> +          for (let winID in this._listeners[aPage.manifest]) {
> +            if (this._listeners[aPage.manifest][winID]) {

Same question here, and use a local for this._listeners[aPage.manifest]

::: dom/messages/SystemMessageManager.js
@@ +122,5 @@
> +
> +    cpmm.sendAsyncMessage("SystemMessageManager:Unregister",
> +        { manifest: this._manifest,
> +          innerWindowID: this.innerWindowID
> +        });

Nit: indent the { either to 2 spaces after cpmm or to "System

Comment 21

[Benjamin Chen [:bechen]]

Attachment: fix system message leak

Carry over r+ after addressing comment 20.

Comment 22

[Benjamin Chen [:bechen]]

try server:
https://tbpl.mozilla.org/?tree=Try&rev=c68c7c8e2979

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Should this have a test?

Comment 24

[(no longer active)]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/782795b933e0