1055761 - Navigating to a URL can incorrectly switch to an existing browser window

Status: RESOLVED FIXED

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

Description

[Ben Francis [:benfrancis]]

STR:
* Enter Rocketbar from homescreen
* Enter "mozilla.org" and hit enter
* Wait for the page to load, scroll to the bottom, click on a link to another web site (e.g. Twitter).
* Go to the homescreen
* Enter Rocketbar
* Type "mozilla.org" and hit enter

Expected:
* mozilla.org is loaded in a new browser window

Actual:
* window manger switches to the browser window containing Twitter.

I suspect this has the same underlying cause as bug 1055723 and is to do with named windows.

Comment 1

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Hey Alive - what do you think about something like this. Right now it just matches against the original origin of the app window. Updating the origin on locationchange seems risky, so just checking on the current app window URLs seems like it may be a better approach.

Comment 2

[Kevin Grandon :kgrandon]

I've also gone and added a marionette test for this which should be extremely useful in the future to catch regressions.

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8475480 [details] [review]
Github pull request

Adding Tim/Etienne in case they would be able to review this as well. Don't want to over-burden Alive.

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8475480 [details] [review]
Github pull request

It would be good if Alive have time to review this as well.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8475480 [details] [review]
Github pull request

I don't think so...

The problem is the mozilla.org browser window has changed its origin,
so if we change the origin in the browser window object and make
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_window_factory.js#L182
returns null(maybe need to fix getApp()), appWindowFactory will new it for you..

Comment 6

[Ben Francis [:benfrancis]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> The problem is the mozilla.org browser window has changed its origin

Sorry if I'm not understanding you correctly, but shouldn't a new navigation to a new URL get its own browser window, even if it is from the same origin as a URL in another browser window?

Comment 7

[Kevin Grandon :kgrandon]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #5)
> Comment on attachment 8475480 [details] [review]
> Github pull request
> 
> I don't think so...
> 
> The problem is the mozilla.org browser window has changed its origin,
> so if we change the origin in the browser window object and make
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/
> app_window_factory.js#L182
> returns null(maybe need to fix getApp()), appWindowFactory will new it for
> you..

I think you mean change getApp() to return null? I think this approach is much more risky and likely cause regressions, but we can try it.

Comment 8

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8477034 [details] [review]
Github pull request

Hey Alive - here is a simplified version of the patch which just change the app_window logic. Should not be too risky because we check on isBrowser(). Let me know what you think, thanks!

Comment 10

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8477034 [details] [review]
Github pull request

Basically ok but I have a more descriptive if statement proposal in github.
And please have unit test as well.

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 8477034 [details] [review]
Github pull request

Alive - I've added a unit test here. I tried using the new pattern, but lots of unit tests started failing. It could potentially be a problem with the tests, but I'd rather not risk the regression here, and just follow the patterns that we currently have in place. Let me know what you think.

Comment 12

[(inactive after 6/18) Alive Kuo [:alive]]

Well, this is not a rushing feature, so let's figure out what's happening then left something we never know why it works in current system. If you don't want to do this please let me know, I will ask other people to help.

Comment 13

[Kevin Grandon :kgrandon]

Hey Alive - the problem is that origin handling is also important, because that's what most things pass into getApp(). We can massage the conditional you supplied to me to include this handling, but I think that's a lot more messy than what's implemented now. In the current pull request each branch is well-defined and it's pretty clear what it's doing. Without changing the arguments to getApp(), we may end up needing to add another branch, like: (app.origin === origin && !manifestURL && !app.isBrowser()).

If we can achieve the same functionality with only passing in a URL/manifestURL to getApp() that might be an option, but this would be a rather risky refactoring for not much gain IMO. My preference is to still go with what is in the PR now, but open to changing it based on your direction. It would just take a lot more work, and I'm not sure if I can do it this week. If someone on your team could take it that would be fine though. What do you think?

Comment 14

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Kevin Grandon :kgrandon from comment #13)
> Hey Alive - the problem is that origin handling is also important, because
> that's what most things pass into getApp(). We can massage the conditional
> you supplied to me to include this handling, but I think that's a lot more
> messy than what's implemented now. In the current pull request each branch
> is well-defined and it's pretty clear what it's doing. Without changing the
> arguments to getApp(), we may end up needing to add another branch, like:
> (app.origin === origin && !manifestURL && !app.isBrowser()).

Add one more statement works for me, though !app.isBrowser() && !manifestURL could not happen at the same time.

> 
> If we can achieve the same functionality with only passing in a
> URL/manifestURL to getApp() that might be an option, but this would be a
> rather risky refactoring for not much gain IMO. My preference is to still go
> with what is in the PR now, but open to changing it based on your direction.
> It would just take a lot more work, and I'm not sure if I can do it this
> week. If someone on your team could take it that would be fine though. What
> do you think?

If it's your promise I am fine.

I am struggling for the maintenance of system app, and it's definitely not 'not much gain'.
I had stated my opinion here.

Comment 15

[Kevin Grandon :kgrandon]

Hey - just so we are on the same page. The argument, 'manifestURL' is null because we don't pass it into the getApp() call. Here is a list of usage where we only pass origin, and not manifestURL: https://gist.github.com/KevinGrandon/ebe14cf7b30b76897d7c . Because of this swapping the operators within the if() statement makes it much more complex because we need another branch. Without changing arguments, one working example (using your new style and the additional branch) is: 

if ((app.origin === origin && !manifestURL && !app.isBrowser()) ||
(manifestURL && app.manifestURL === manifestURL) ||
(app.isBrowser() && app.config.url === origin)) {


(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> If it's your promise I am fine.
> 
> I am struggling for the maintenance of system app, and it's definitely not
> 'not much gain'.
> I had stated my opinion here.

Sorry if I'm not clear, I would definitely like to land in a way were we feel comfortable for 2.1, so we have ~1 week here. I do think this will change dramatically when we go to do some more work on "app scope" and we can clean it up then, but I definitely want us both to be happy before we land this for 2.1.

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Kevin Grandon :kgrandon from comment #15)
> Hey - just so we are on the same page. The argument, 'manifestURL' is null
> because we don't pass it into the getApp() call. Here is a list of usage
> where we only pass origin, and not manifestURL:
> https://gist.github.com/KevinGrandon/ebe14cf7b30b76897d7c . Because of this
> swapping the operators within the if() statement makes it much more complex
> because we need another branch. Without changing arguments, one working
> example (using your new style and the additional branch) is: 
> 
> if ((app.origin === origin && !manifestURL && !app.isBrowser()) ||
> (manifestURL && app.manifestURL === manifestURL) ||
> (app.isBrowser() && app.config.url === origin)) {

Hmm...I think it's the design problem of the function, what we could improve is:
so please with this and let us improve the function call later.

> 
> 
> (In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> > If it's your promise I am fine.
> > 
> > I am struggling for the maintenance of system app, and it's definitely not
> > 'not much gain'.
> > I had stated my opinion here.
> 
> Sorry if I'm not clear, I would definitely like to land in a way were we
> feel comfortable for 2.1, so we have ~1 week here. I do think this will
> change dramatically when we go to do some more work on "app scope" and we
> can clean it up then, but I definitely want us both to be happy before we
> land this for 2.1.

Comment 18

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #17)
> (In reply to Kevin Grandon :kgrandon from comment #15)
> > Hey - just so we are on the same page. The argument, 'manifestURL' is null
> > because we don't pass it into the getApp() call. Here is a list of usage
> > where we only pass origin, and not manifestURL:
> > https://gist.github.com/KevinGrandon/ebe14cf7b30b76897d7c . Because of this
> > swapping the operators within the if() statement makes it much more complex
> > because we need another branch. Without changing arguments, one working
> > example (using your new style and the additional branch) is: 
> > 
> > if ((app.origin === origin && !manifestURL && !app.isBrowser()) ||
> > (manifestURL && app.manifestURL === manifestURL) ||
> > (app.isBrowser() && app.config.url === origin)) {
> 
> Hmm...I think it's the design problem of the function, what we could improve
> is:
> so please with this and let us improve the function call later.
> 

Sorry for my sentence, I mean I could r+ this one!

Comment 19

[Kevin Grandon :kgrandon]

Comment on attachment 8477034 [details] [review]
Github pull request

Hey Alive - Marking you for review again. Please let me know if I should go with what is in the PR, or comment 15 and I will update - I'm not sure which one you like better right now.

Also please file a follow-up bug if you would like us to make this better in the future. Thank you.

Comment 20

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8477034 [details] [review]
Github pull request

Much better now, thanks.

Comment 21

[Kevin Grandon :kgrandon]

Master: https://github.com/mozilla-b2g/gaia/commit/75fedbe72fcadb32ecea9e074d5f114eba2535e9