All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED

Status

P3
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jj.evelyn, Assigned: bechen)

Tracking

unspecified
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

6 years ago
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.
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?
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Updated

6 years ago
Summary: Can't get window.location in inline disposition web activity when it share the same view of an opened app → Can't set window.location in inline disposition web activity when its App frame is opened
Is there a workaround we can use?  What activity do we need this for?
Flags: needinfo?(ehung)
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.
(Reporter)

Comment 5

6 years ago
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.
Flags: needinfo?(ehung)
If you determine this is a bug, Evelyn, please clear the needinfo flag and we'll discuss it again in triage.  Thanks.
Flags: needinfo?(ehung)
(Reporter)

Comment 7

6 years ago
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.
Flags: needinfo?(ehung)
Duplicate of this bug: 805734

Comment 9

6 years ago
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

6 years ago
Created attachment 675551 [details]
Screenshot
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 :)
Assignee: nobody → clian
blocking-basecamp: ? → +
Priority: -- → P3
(Assignee)

Updated

6 years ago
Assignee: clian → bechen
Hi Benjamin, thanks for volunteering taking this! I'm glad to discuss with you about activities/system-messages mechanisms at any time. ;)
(Assignee)

Comment 13

6 years ago
Created attachment 677262 [details] [diff] [review]
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"
Attachment #677262 - Flags: feedback?(fabrice)
(Assignee)

Comment 14

6 years ago
Created attachment 677275 [details] [diff] [review]
fix system message leak

check aTopic in DOMRequestIpcHelper observe method
Attachment #677262 - Attachment is obsolete: true
Attachment #677262 - Flags: feedback?(fabrice)
Attachment #677275 - Flags: feedback?(fabrice)
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.
Attachment #677275 - Flags: feedback?(fabrice) → feedback+
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...
Attachment #677275 - Flags: feedback+
(Assignee)

Comment 17

6 years ago
Created attachment 677323 [details] [diff] [review]
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
Attachment #677275 - Attachment is obsolete: true
Attachment #677323 - Flags: review?(fabrice)
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.
Attachment #677323 - Flags: review?(fabrice) → review-
(Assignee)

Comment 19

6 years ago
Created attachment 678199 [details] [diff] [review]
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.
Attachment #677323 - Attachment is obsolete: true
Attachment #678199 - Flags: review?(fabrice)
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
Attachment #678199 - Flags: review?(fabrice) → review+
(Assignee)

Comment 21

6 years ago
Created attachment 679029 [details] [diff] [review]
fix system message leak

Carry over r+ after addressing comment 20.
Attachment #678199 - Attachment is obsolete: true
Attachment #679029 - Flags: review+
(Assignee)

Comment 22

6 years ago
try server:
https://tbpl.mozilla.org/?tree=Try&rev=c68c7c8e2979
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e56c5e24b0d1

Should this have a test?
Flags: in-testsuite?
Keywords: checkin-needed

Comment 24

6 years ago
https://hg.mozilla.org/mozilla-central/rev/e56c5e24b0d1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 806959
https://hg.mozilla.org/releases/mozilla-aurora/rev/782795b933e0
status-firefox18: --- → fixed
status-firefox19: --- → fixed
You need to log in before you can comment on or make changes to this bug.