Anshul Reporter
	Description • 10 years ago

[Blocking Requested - why for this release]:

We are seeing this exception during our marionette based unit tests on  FFOS 2.1 

There is no specific step that causes this error so I don't have an STR except that this crash happens mostly when running voice call related tests. This is breaking our unit test, which is our way to ensure there are no regressions due to code churn happening on a branch.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 1 • 10 years ago

I'll look into this

	Gregor Wagner [:gwagner]
	Comment 2 • 10 years ago

bug

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 3 • 10 years ago

The line in question is in taskManager.exitToApp, where we assume the .unfilteredStack property is an array of AppWindow instances given by StackManager.snapshot() when the taskManager was shown. I've not been able to reproduce this issue, but I think we can head it off quite simply as the access to this.unfilteredStack is used to establish if the position in the stack has changed. In the case of an attention screen, we dont want to change the stack position so a simple guard for a falsey unfilteredStack should do the job. (Though this doesn't help us understand how we arrive in this state to begin with.)

Anshul, would it be possible to see this marionette test? Maybe we can add something similar to our test suite to try and ensure this is fixed and stays fixed. Meantime I'll get that master and 2.1 patches on this bug

	Anshul Reporter
	Comment 6 • 10 years ago

(In reply to Sam Foster [:sfoster] from comment #3)
> The line in question is in taskManager.exitToApp, where we assume the
> .unfilteredStack property is an array of AppWindow instances given by
> StackManager.snapshot() when the taskManager was shown. I've not been able
> to reproduce this issue, but I think we can head it off quite simply as the
> access to this.unfilteredStack is used to establish if the position in the
> stack has changed. In the case of an attention screen, we dont want to
> change the stack position so a simple guard for a falsey unfilteredStack
> should do the job. (Though this doesn't help us understand how we arrive in
> this state to begin with.)
> 
> Anshul, would it be possible to see this marionette test? Maybe we can add
> something similar to our test suite to try and ensure this is fixed and
> stays fixed. Meantime I'll get that master and 2.1 patches on this bug
Sam, there isn't a specific test that is causing this issue so sharing them would only confuse everyone. I can help verify the patch to see if it fixes the issue after I am able to manually reproduce it.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 7 • 10 years ago

Anshul, if you could try out the patch that would be a help, I dont think we've seen anything like this failure in our testsuite, though it is still fairly early days.

	Etienne Segonzac (:etienne)
	Comment 8 • 10 years ago

Comment on attachment 8488127 [details]
v2.1 patch - Guard against null unfilteredStack in TaskManager's exitToApp

let's go with this for 2.1

	Etienne Segonzac (:etienne)
	Comment 9 • 10 years ago

Comment on attachment 8488124 [details]
master patch - Guard against null unfilteredStack in TaskManager's exitToApp

For master I'd really like to know how to reproduce it locally.

The thing is, we want to add a test for this, to make sure it won't happen again. And if we don't know how it can happen, we can't add a test to make sure it never happens again.

Anshul, is there any manual scenario we can do on our side to get this error?

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 10 • 10 years ago

Comment on attachment 8488127 [details]
v2.1 patch - Guard against null unfilteredStack in TaskManager's exitToApp

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 1058722
[User impact] if declined: Partner smoke(?) test blocker. No known user impact
[Testing completed]: Manual testing on 2.1
[Risk to taking this patch] (and alternatives if risky): V. low risk
[String changes made]: None

	Anshul Reporter
	Comment 11 • 10 years ago

(In reply to Etienne Segonzac (:etienne) from comment #9)
> Comment on attachment 8488124 [details]
> master patch - Guard against null unfilteredStack in TaskManager's exitToApp
> 
> For master I'd really like to know how to reproduce it locally.
> 
> The thing is, we want to add a test for this, to make sure it won't happen
> again. And if we don't know how it can happen, we can't add a test to make
> sure it never happens again.
> 
> Anshul, is there any manual scenario we can do on our side to get this error?

Etienne, on the recent FFOS 2.1 builds I am noticing that the phone app is not displayed on the homescreen at all. I need to make an incoming call first to get the phone app running before I can make a call.

The exception seems to happen if I make an outgoing call as soon as the phone bootup without having any incoming call first. Since the phone app isn't seen at all, I am not able to manually reproduce the scenario.

FYI, your patch seems to be fixing the issue. However now I am seeing error below in some other script, which seems like a different issue.

JavascriptException: JavascriptException: TypeError: window.getComputedStyle(...) is null at: app://callscreen.gaiamobile.org/gaia_build_defer_index.js line: 144

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 12 • 10 years ago

> FYI, your patch seems to be fixing the issue. However now I am seeing error
> below in some other script, which seems like a different issue.
> 
> JavascriptException: JavascriptException: TypeError:
> window.getComputedStyle(...) is null at:
> app://callscreen.gaiamobile.org/gaia_build_defer_index.js line: 144

These look like distinct and probably unrelated bugs. If you can reproduce them could you file new bugs? The gaia_build_defer_index.js is the aggregate and minified file, it would help to get a log using a non-production build which will give us a more useful file and line number in the exception. 

For the "unfilteredStack is null" issue, this bug is marked to block 2.1. We've got what looks to be an effective fix on the 2.1 branch, but no STR to write a test to land on master. As it stands, we cant ship 2.1 without closing this bug. In the absence of those STR we seem to be stuk here. I dont want to lose track of this, if we dont get the same fix on master we may run into the same issue in 2.2. Perhaps we should just land the patch and examine our integration tests to confirm that an exception like this would be caught?

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 13 • 10 years ago

What worries me is that these marionette tests seem to be running into race conditions that we cant reproduce. This is a variable that will keep throwing up these head-scratching issues until we pin it down.

	Anshul Reporter
	Comment 14 • 10 years ago

(In reply to Sam Foster [:sfoster] from comment #12)
> > FYI, your patch seems to be fixing the issue. However now I am seeing error
> > below in some other script, which seems like a different issue.
> > 
> > JavascriptException: JavascriptException: TypeError:
> > window.getComputedStyle(...) is null at:
> > app://callscreen.gaiamobile.org/gaia_build_defer_index.js line: 144
> 
> These look like distinct and probably unrelated bugs. If you can reproduce
> them could you file new bugs? The gaia_build_defer_index.js is the aggregate
> and minified file, it would help to get a log using a non-production build
> which will give us a more useful file and line number in the exception. 
Sam by non production builds, do you mean making an eng flavor of the build?

> 
> For the "unfilteredStack is null" issue, this bug is marked to block 2.1.
> We've got what looks to be an effective fix on the 2.1 branch, but no STR to
> write a test to land on master. As it stands, we cant ship 2.1 without
> closing this bug. In the absence of those STR we seem to be stuk here. I
> dont want to lose track of this, if we dont get the same fix on master we
> may run into the same issue in 2.2. Perhaps we should just land the patch
> and examine our integration tests to confirm that an exception like this
> would be caught?

	Etienne Segonzac (:etienne)
	Comment 15 • 10 years ago

Comment on attachment 8488124 [details]
master patch - Guard against null unfilteredStack in TaskManager's exitToApp

Yeah, I agree that keeping this patch out of master isn't helping use in any way...

r=me with the small comment addressed and a unit test purposefully calling exitToApp() with something that's not part of the stack checking that nothing explodes :)

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 16 • 10 years ago

> Sam by non production builds, do you mean making an eng flavor of the build?

That's right.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 17 • 10 years ago

Comment on attachment 8488124 [details]
master patch - Guard against null unfilteredStack in TaskManager's exitToApp

PR updated. After rebasing I had to touch the logic in exitToApp. I Added the requested unit tests and backfilled a little on the filtering unit tests. Re-flagging for review to get a 2nd pair of eyes on the tests and logic changes.

	Etienne Segonzac (:etienne)
	Comment 18 • 10 years ago

Comment on attachment 8488124 [details]
master patch - Guard against null unfilteredStack in TaskManager's exitToApp

Kudos on the added test coverage.

But I just realized I was wrong earlier :/
|this.position| refers to the position in the potentially filtered task, which means:

* we should lookup this.position in this.stack and not in this.unfliteredStack (for the exitToApp() case when we press the home button)

* in our bailout case where unfilteredStack is weirdly null, we should take StackManager.position like you originally did and not this.position (sorry)

Otherwise pressing home from a filtered stack will potentially send you to an open app instead of a web sheet.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 19 • 10 years ago

(In reply to Etienne Segonzac (:etienne) from comment #18)
> Comment on attachment 8488124 [details]

> 
> But I just realized I was wrong earlier :/
> |this.position| refers to the position in the potentially filtered task,
> which means:
> 
> * we should lookup this.position in this.stack and not in
> this.unfliteredStack (for the exitToApp() case when we press the home button)
> 
> * in our bailout case where unfilteredStack is weirdly null, we should take
> StackManager.position like you originally did and not this.position (sorry)

yeah I had to look twice at that. I guess if this.unfilteredStack is null, its possible this.position and this.stack is also undefined. In which case StackManager.position should be our fallback. Somehow exitToApp must be getting called before show(). Maybe their tests fire taskmanagershow or holdhome but don't wait for cardviewshown? Anyhow I'll update the PR.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 20 • 10 years ago

Comment on attachment 8488124 [details]
master patch - Guard against null unfilteredStack in TaskManager's exitToApp

PR updated for master. I want to double-check the exist/homebutton logic here as I ended up touching some of that. Francis, can you checkout the branch on https://github.com/mozilla-b2g/gaia/pull/23967 and verify that the home button does what you expect in both browser-windows and normal apps modes, when you close sheets, close the active sheet, move around the strip etc. 

IIRC what we agreed and what this patch should explicitly implement is that if you close the active sheet in task-manager, the home button will drop you back to the homescreen. Otherwise it should take you back to the active app - the one you were on when you launched the task manager. 

I'll be updating the 2.1 patch too if this logic is ok. We didnt get the strip-style task manager in 2.1, but we did get this same return-to-active-app behavior in.

	Francis Djabri [:djabber]
	Comment 21 • 10 years ago

Comment on attachment 8488124 [details]
master patch - Guard against null unfilteredStack in TaskManager's exitToApp

There are a couple of things that don't match the expected behavior. First, the animation when returning to the active card when pressing the home key is missing: 

https://mozilla.box.com/s/uxs4c3cf504nt4v3cz7a 

Secondly, when deleting a card, the card should be replaced by the card on the left, not the card on the right.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 22 • 10 years ago

(In reply to Francis Djabri [:djabber] from comment #21)
> Comment on attachment 8488124 [details]
> master patch - Guard against null unfilteredStack in TaskManager's exitToApp
> 
> There are a couple of things that don't match the expected behavior. First,
> the animation when returning to the active card when pressing the home key
> is missing: 
> 
> https://mozilla.box.com/s/uxs4c3cf504nt4v3cz7a 

I've opened bug 1071326 to track this - this is the behavior as-is on master currently. I also opened bug 1071321 for the case where the active app has been closed. 

> Secondly, when deleting a card, the card should be replaced by the card on
> the left, not the card on the right.

I opened bug 1071320 to track this. 

I'll rework the patch to avoid confusing or exacerbating these problems and confine the scope to the unfilteredStack is null problem as much as is possible. That should remove the need for ui-review.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 23 • 10 years ago

Comment on attachment 8488124 [details]
master patch - Guard against null unfilteredStack in TaskManager's exitToApp

Clearing UI-review as we are tracking these issues in other bugs now. 

The PR has been updated. The code in question has changed since 2.1, so we need separate patches for 2.1 and master. Although this is quite a small and trivial patch against master, we now have bugs open (bug 1071326, bug 1071320) which make it difficult to confirm that a patch to fix this problem in master doesn't regress something else. I could block on those, but as we're waiting for the master patch here to land before landing the 2.1 patch, that makes those 2.1 blockers, which isnt right either. 

As far as I can tell, the logic here in exitToApp does the right thing, and doesnt change the (currently broken) behavior. We fallback to StackManager.getCurrent(), and failing that, the homescreen. We have improved test coverage with this patch. If Gaia-Try comes up green and you dont see any issues in this patch I'd suggest we land it, resolve this bug and handle follow-ups in the existing bugs or new bugs as appropriate.

	Etienne Segonzac (:etienne)
	Comment 24 • 10 years ago

Comment on attachment 8488124 [details]
master patch - Guard against null unfilteredStack in TaskManager's exitToApp

Sorry to be a pain but
- either we keep the master behavior where the centered card zooms in
- or we implement the spec (going back to the previously active app) *with* the transition (bug 1071326)

But I don't want to land the **** behavior where the previously active app appears from nowhere and zooms in, it's too confusing.

The rest of the patch is good, so I'd vote to keep the current behavior and do the rest independently in bug 1071326.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 25 • 10 years ago

(In reply to Etienne Segonzac (:etienne) from comment #24)
> But I don't want to land the half-assed behavior where the previously active
> app appears from nowhere and zooms in, it's too confusing.

The current behavior to open whatever app is centered when we tap home is not to spec. But that is for another bug. I'll update the PR to leave this as-is and just guard against falsey this.unfilteredStack and this.stack. 
 
> 
> The rest of the patch is good, so I'd vote to keep the current behavior and
> do the rest independently in bug 1071326.

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 26 • 10 years ago

Comment on attachment 8488124 [details]
master patch - Guard against null unfilteredStack in TaskManager's exitToApp

Revised patch leaving exitToApp as-is - only referencing this.stack instead of unfilteredStack. In the weird case where these are not defined, app should be the homescreen.

If we need to iterate further on this, I suggest we open a follow up. The 2.1 patch needs to land.

	bhavana bajaj [:bajaj]
	Comment 27 • 10 years ago

Still waiting for final reviews and master landing to happen here before getting to branch approval.

	Etienne Segonzac (:etienne)
	Comment 28 • 10 years ago

Comment on attachment 8488124 [details]
master patch - Guard against null unfilteredStack in TaskManager's exitToApp

Thanks!

	Sam Foster [:sfoster] (he/him) Assignee
	Comment 29 • 10 years ago

Try is green, merged to master: https://github.com/mozilla-b2g/gaia/commit/a8e4d164e64c6a618e4093eb53068de4ad74dfa9
Commit: https://github.com/mozilla-b2g/gaia/commit/efa7274a91911922d4f36d7604ab7a566cf72be3

Note: don't uplift the master patch, use attachment 8488127 [details] for 2.1

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 10 years ago

v2.1: https://github.com/mozilla-b2g/gaia/commit/6ef1018c0d58774ca32bb7ac0059784f6deb6868

	Kevin Grandon :kgrandon
	Comment 31 • 10 years ago

The unit tests that landed with the original patch should suffice for now. Let's not worry about adding integration tests until we do the task manager work in 2.2.