1064712 - [JavaScript Error: "TypeError: element is null" {file: "app://system.gaiamobile.org/js/task_manager.js" line: 474}]

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect, P1)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

JS error while killing app.

Steps to reproduce:

1. Open any app
2. Long tap  home button to bring up card view
3. Swipe to kill the app

In adb logcat
[JavaScript Error: "TypeError: element is null" {file: "app://system.gaiamobile.org/js/task_manager.js" line: 474}]

Comment 1

[Kan-Ru Chen [:kanru] (UTC+9)]

Gaia      b630b8bcaf9653885539d4449bc65c3b592bd752
Gecko    
BuildID   20140909144121
Version   35.0a1
ro.build.version.incremental=96
ro.build.date=Fri May 23 11:17:40 CST 2014
B1TC400110G0

Comment 2

[Sam Foster [:sfoster] (he/him)]

Can you re-test on a nightly/recent gaia? I believe this was fixed by bug 1061324.

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

Confirmed fixed.

Comment 4

[Tony Chung [:tchung]]

reopening bug.  I am seeing this on aurora builds today.   Sam, possible regression or bug 1061324 really didnt fix it?

Repro:
1) install 2.1 aurora KK build on Flame KK

Gaia-Rev        6a6ed9433fce47e76c07fd35bc5952acb108f4c8
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/7b09a378588c
Build-ID        20140926000202
Version         34.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20140925.032655
FW-Date         Thu Sep 25 03:27:06 EDT 2014
Bootloader      L1TC10011800

2) launch any app
3) long press to open task switcher
4) swipe to kill
5) Verify logcat always shows:
09-26 15:04:52.579: E/GeckoConsole(204): [JavaScript Error: "TypeError: element is null" {file: "app://system.gaiamobile.org/js/task_manager.js" line: 481}]

Comment 5

[Sam Foster [:sfoster] (he/him)]

(In reply to Tony Chung [:tchung] from comment #4)
> reopening bug.  I am seeing this on aurora builds today.   Sam, possible
> regression or bug 1061324 really didnt fix it?

We've had other task manager changes land in the meantime, we could track down exactly what regressed but I think it would be more productive to just land a 2.1 patch to fix it. I'm attaching a PR.

Comment 6

[Sam Foster [:sfoster] (he/him)]

Attachment: v2.1 - Dont call taskManager.removeCard twice

Comment 7

[Sam Foster [:sfoster] (he/him)]

Watching try results at: https://tbpl.mozilla.org/?rev=766e9c19b896f801bbc7e5f12736493e3ce81cf1&tree=Gaia-Try
But I'm not sure we have coverage for this, I'll also try it on device later today.

Comment 8

[Tony Chung [:tchung]]

[Blocking Requested - why for this release]:

Thanks!  i'll request 2.1 blocking for uplift if you can help justify that this is a safe and effective fix.

Comment 9

[Sam Foster [:sfoster] (he/him)]

I'm going to pin down exactly when this happens to know if it has real impact that would justify blocking.

Comment 10

[Sam Foster [:sfoster] (he/him)]

In TaskManager, either the close button or swipe gesture ends up calling taskManager.closeApp(). This first kills the appWindow instance assocated with that card, then removes the card via removeCard(). The exception occurs because calling card.killApp calls AppWindow kill, which generates an appterminated event.
appterminated is handled by TaskManager (in case an app is killed by some other means while the task manager is showing) and it too calls removeCard. So either removeCard should guard against removing an already-removed card (might be prudent in any case) or closeApp can also skip calling removeCard as it will be removed by the appterminated event handler. 

As far as user-impact goes, removeCard call in closeApp is raising an exception while trying to access properties on objects that no longer exist. The card is already removed, and this is the last operation in the clsoeApp method - so though its messy and partners may also spot the logged exception - there should be no other user impact.

Comment 11

[Gregor Wagner [:gwagner]]

Not blocking based on comment 10 but please request uplift approval.

Comment 12

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8496291 [details] [review]
v2.1 - Dont call taskManager.removeCard twice

I've updated the PR. In master, the extra call to removeCard in the closeApp (renamed killApp) method has already been removed. I've made an equivalent change with a little clean-up/hardening. There's really nothing to do on master here - this issue does not reproduce in that branch.

Comment 13

[Etienne Segonzac (:etienne)]

Comment on attachment 8496291 [details] [review]
v2.1 - Dont call taskManager.removeCard twice

small comment on github we should look after, but not blocking

Comment 14

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8496291 [details] [review]
v2.1 - Dont call taskManager.removeCard twice

Just updating attachment title to reflect what the patch does. Also updated the PR to remove the unused 2nd param to removeCard.

Comment 15

[Sam Foster [:sfoster] (he/him)]

Attachment: v2.1 Dont call Task Manager removeCard twice when closing a card

New pull request as the other one got closed. Carrying etienne's r+

Comment 16

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8499203 [details] [review]
v2.1 Dont call Task Manager removeCard twice when closing a card

Approval Request Comment
[Feature/regressing bug #]: Closing cards in Task Manager
[User impact if declined]: No user-visible impact that I'm aware of. The first call to removeCard has already done the work. But it might cause concern and confusion
[Describe test coverage new/current, TBPL]: New unit tests to augment existing. This feature is also covered by python integration tests. Manual testing on device.
[Risks and why]: Very low risk. No impact outside the task manager, Code hardening and clean-up only
[String/UUID change made/needed]: None

Comment 17

[Sam Foster [:sfoster] (he/him)]

Fabrice, is this on your radar? There is no equivalent patch for master - it was fixed in bug 1061324 which did not uplift. So it doesnt make sense to resolve this until the 2.1 patch lands (or not), and my impression is that the approval-required filters require a bug to either be resolved/fix or blocking. Sorry for the noise if not.

Comment 18

[[:fabrice] Fabrice Desré]

Sam, this is a gaia patch so you need to ask approval-gaia-v2.1, no approval-aurora on the patch!

Comment 19

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8499203 [details] [review]
v2.1 Dont call Task Manager removeCard twice when closing a card

Oops, thanks Fabrice I hadn't noticed the different approval flags for gaia

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Closing cards in Task Manager
[User impact] if declined: No user-visible impact that I'm aware of. The first call to removeCard has already done the work. The logged exception may cause concern and confusion
[Testing completed]: New unit tests to augment existing. This feature is also covered by python integration tests. Manual testing on device.
[Risk to taking this patch] (and alternatives if risky): Very low risk. No impact outside the task manager, Code hardening and clean-up only. Alternative is to live with the logged exception
[String changes made]: None

Comment 20

[Sam Foster [:sfoster] (he/him)]

Merged to 2.1: https://github.com/mozilla-b2g/gaia/commit/aa2b95701d23c3d2f5958896f50c1fcebca98c5f

Comment 21

[Alissa（leave from Mozilla）]

Attachment: logcat of flame 2.1

Please check the logcat.
steps:
1.Launch an app.
2.Long tap home key and swip to kill it.

FLame2.1 new build:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141201001201
Version         34.0

Comment 22

[Alissa（leave from Mozilla）]

Hi,tony
Could you help to check the logcat agian,I have not found java error in logcat.

Comment 23

[Tony Chung [:tchung]]

(In reply to Alissa from comment #22)
> Hi,tony
> Could you help to check the logcat agian,I have not found java error in
> logcat.

Hi Alissa, your verification in comment 21 is good enough. Next time, please be clear in your comment that you cannot reproduce the issue anymore after the fix.  You can now safely mark this bug as status-b2g-v2.1 == "verified", since the patch was merged on 2.1 back on comment 20.

Comment 24

[Tony Chung [:tchung]]

(In reply to Tony Chung [:tchung] from comment #23)
> (In reply to Alissa from comment #22)
> > Hi,tony
> > Could you help to check the logcat agian,I have not found java error in
> > logcat.
> 
> Hi Alissa, your verification in comment 21 is good enough. Next time, please
> be clear in your comment that you cannot reproduce the issue anymore after
> the fix.  You can now safely mark this bug as status-b2g-v2.1 == "verified",
> since the patch was merged on 2.1 back on comment 20.

Alissa, you should only mark the Status -> "VERIFIED" if you have tested this patch against mozilla-central builds.  So i'm going to move this back to RESOLVED FIXED until you have done so.   please make sure you have done that also, and log your test environment in this bug comment.   For status-b2g-v2.1 -> verified, that only applies if you have tested this against a 2.1 build.

Thanks.

Comment 25

[Alissa（leave from Mozilla）]

This issue has been failed verified on Flame 2.1
See attachment:logcat.txt
Reproducing rate: 5/5

Flame 2.1 new build:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141202001201
Version         34.0
-------------------
Hi,tony
I check the logcat agian,java error still exits,so recover status-b2g-v2.1.

Comment 26

[Alissa（leave from Mozilla）]

Attachment: 20141202001201logcat

Comment 27

[Alissa（leave from Mozilla）]

This issue has been failed verified on Flame 2.1
See attachment:logcat.txt
Reproducing rate: 5/5

Flame 2.1 new build:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141202001201
Version         34.0
-------------------
Hi,tony
I check the logcat agian,java error still exits,so recover status-b2g-v2.1.