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

RESOLVED FIXED in 2.1 S3 (29aug)

Status

Firefox OS
Gaia::System::Window Mgmt
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: benfrancis, Assigned: kgrandon)

Tracking

unspecified
2.1 S3 (29aug)
x86_64
Linux

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe])

Attachments

(1 attachment, 1 obsolete attachment)

46 bytes, text/x-github-pull-request
alive
: review+
alive
: feedback+
Details | Review | Splinter Review
(Reporter)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
See Also: → bug 1045894
(Assignee)

Comment 1

4 years ago
Created attachment 8475480 [details] [review]
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.
Assignee: nobody → kgrandon
Status: NEW → ASSIGNED
Attachment #8475480 - Flags: review?(alive)
(Assignee)

Comment 2

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

Comment 3

4 years ago
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.
Attachment #8475480 - Flags: review?(timdream)
Attachment #8475480 - Flags: review?(etienne)
Comment on attachment 8475480 [details] [review]
Github pull request

It would be good if Alive have time to review this as well.
Attachment #8475480 - Flags: review?(timdream) → review+
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..
Attachment #8475480 - Flags: review?(alive)
Attachment #8475480 - Flags: review-
Attachment #8475480 - Flags: review+
Attachment #8475480 - Flags: review?(etienne)
(Reporter)

Comment 6

4 years ago
(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?
(Assignee)

Comment 7

4 years ago
(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.
(Assignee)

Comment 8

4 years ago
Created attachment 8477034 [details] [review]
Github pull request
(Assignee)

Updated

4 years ago
Attachment #8475480 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
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!
Attachment #8477034 - Flags: review?(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.
Attachment #8477034 - Flags: review?(alive) → feedback+
(Assignee)

Comment 11

4 years ago
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.
Attachment #8477034 - Flags: review?(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.
(Assignee)

Comment 13

4 years ago
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?
Flags: needinfo?(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.
Flags: needinfo?(alive)
(Assignee)

Comment 15

4 years ago
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.
Flags: needinfo?(alive)
(Reporter)

Updated

4 years ago
Duplicate of this bug: 1055723
(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.
Flags: needinfo?(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!
(Assignee)

Comment 19

4 years ago
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.
Attachment #8477034 - Flags: review?(alive)
Comment on attachment 8477034 [details] [review]
Github pull request

Much better now, thanks.
Attachment #8477034 - Flags: review?(alive) → review+
(Assignee)

Comment 21

4 years ago
Master: https://github.com/mozilla-b2g/gaia/commit/75fedbe72fcadb32ecea9e074d5f114eba2535e9
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [systemsfe]
Target Milestone: --- → 2.1 S3 (29aug)
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1045894
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1058784
You need to log in before you can comment on or make changes to this bug.