961157 - Tiles not working/opening www.metrobrowser.com if firefox not yet running and profilemanager popping up (not auto-use of last profile)

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: General, defect, P2)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox Nightly 29.0a1 2014-01-17 on Windows 8.1 Pro 64 bit

I have set in the profile manager of Firefox that it shouldn't automatically use the profile which has been used the last time (my shortcuts provide the profile name).

Opening a tile added to the Metro home page by pinning a page in Firefox Metro launches the profile manager and after selecting the profile, "metrobrowser" can be seen in the location bar, later auto-fixed to www.metrobrowser.com because "metrobrowser" is the first argument of the application shortcut like e.g.

"C:\Program Files (x86)\Browser\Firefox\unstable\central-de\firefox.exe" metrobrowser -url http://www.mozilla.org/en-US/about/

Comment 1

[juan becerra [:juanb]]

In my case, when I tap on a pinned tile, while I am in desktop mode, the action opens the site in one tab and it also opens www.metrobrowser.com in another tab. I only have a default profile. When I am in metro mode and tap one of these pinned tiles, only the intended site tab opens.

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1

The attached patch fixes the issue for newly-created pinned tiles. The patch simply removes any/all instances of "metrobrowser" appearing in our command lines.

I'm guessing that we used to put "metrobrowser" in the command line so the CEH or maybe nsAppRunner would know to launch metro instead of desktop. Our expected/desired behavior is different now; we launch whatever mode was most recently running. Additionally, there doesn't seem to be any code that knows what to do with "metrobrowser" appearing in the command line.

:bbondy - I've heard that you might know how/why we use "metrobrowser" in our command lines. Will there be an issue if we simply don't add that to our command lines anymore?

Comment 3

[Brian R. Bondy [:bbondy]]

(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #2)
> Created attachment 8364597 [details] [diff] [review]
> Patch v1
> 
> The attached patch fixes the issue for newly-created pinned tiles. The patch
> simply removes any/all instances of "metrobrowser" appearing in our command
> lines.
> 
> I'm guessing that we used to put "metrobrowser" in the command line so the
> CEH or maybe nsAppRunner would know to launch metro instead of desktop. Our
> expected/desired behavior is different now; we launch whatever mode was most
> recently running. Additionally, there doesn't seem to be any code that knows
> what to do with "metrobrowser" appearing in the command line.
> 
> :bbondy - I've heard that you might know how/why we use "metrobrowser" in
> our command lines. Will there be an issue if we simply don't add that to our
> command lines anymore?

I think it was originally used for the reason of front end detection (really early on), but I don't think it's used anymore. So it should be removed.

Comment 4

[Brian R. Bondy [:bbondy]]

Comment on attachment 8364597 [details] [diff] [review]
Patch v1

Review of attachment 8364597 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks good but I didn't test it.
Pls make sure to try these things...

Search contract:
 - Search while no browser is open and metro was the last used
 - Search while no browser is open and desktop was the last used
 - Search while metro browser is open
 - Search while desktop browser is open

Activation secondary tile:
 - while no browser is open and metro was the last used
 - while no browser is open and desktop was the last used
 - while metro browser is open
 - while desktop browser is open

Comment 5

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

>  - Search while no browser is open and metro was the last used
>  - Search while no browser is open and desktop was the last used

No search option is available when no browser is open (confirmed in Nightly)

>  - Search while metro browser is open

Searching seemed to have no effect. Opening the error console revealed this error, followed by a handful of others:

[Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsICommandLine.resolveURI]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: file:///c:/src/mc2/obj/dist/bin/metro/components/BrowserCLH.js :: resolveURIInternal :: line 25"  data: no]
file:///c:/src/mc2/obj/dist/bin/metro/components/BrowserCLH.js
25

I'll need to investigate and fix this issue before landing.

>  - Search while desktop browser is open

No search option is available when only desktopFx is open (confirmed in Nightly)

> Activation secondary tile:
>  - while no browser is open and metro was the last used

Opened metroFx with my previous session (in accordance with my pref) and also a new tab with the pinned URL. However, the focused/selected tab was the tab from my previous session instead of the pinned URL. I tested with Nightly and this did not happen, so this is another issue introduced by this patch that will have to be addressed before landing.

>  - while no browser is open and desktop was the last used

Opened desktopFx with my previous session (in accordance with my pref) and also a new tab with the pinned URL

>  - while metro browser is open

Switched to metroFx and opened pinned URL in a new tab

>  - while desktop browser is open

Switched to desktopFx and opened pinned URL in a new tab

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

> >  - Search while metro browser is open
> 
> Searching seemed to have no effect. Opening the error console revealed this
> error, followed by a handful of others:
> 
> [Exception... "Component returned failure code: 0x804b000a
> (NS_ERROR_MALFORMED_URI) [nsICommandLine.resolveURI]"  nsresult: "0x804b000a
> (NS_ERROR_MALFORMED_URI)"  location: "JS frame ::
> file:///c:/src/mc2/obj/dist/bin/metro/components/BrowserCLH.js ::
> resolveURIInternal :: line 25"  data: no]
> file:///c:/src/mc2/obj/dist/bin/metro/components/BrowserCLH.js
> 25
> 
> I'll need to investigate and fix this issue before landing.

Further investigation has revealed that BrowserCLH's "handle" function is not seeing the entire command line; the first argument is being removed, so instead of handling a command line of "-search search_term," we are handling a command line of "search_term." Placing any argument before the "-search" in MetroContracts.cpp fixes the issue (I used the value "dummy" to test).

> > Activation secondary tile:
> >  - while no browser is open and metro was the last used
> 
> Opened metroFx with my previous session (in accordance with my pref) and
> also a new tab with the pinned URL. However, the focused/selected tab was
> the tab from my previous session instead of the pinned URL. I tested with
> Nightly and this did not happen, so this is another issue introduced by this
> patch that will have to be addressed before landing.

I suspect that this has a similar cause as the above: Probably what's happening is that metroFx is not seeing the "-url" part of the command line; it is just seeing the url on the command line.


I haven't yet tracked down where we're stripping the first command line argument. That's the next step of the investigation.

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

This patch addresses the issues I discovered while testing the previous patch. Brian - do you mind taking another look?

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 8368026 [details] [diff] [review]
Patch v2

Review of attachment 8368026 [details] [diff] [review]:
-----------------------------------------------------------------

Didn't see any problem with secondary tiles with both the desktop and metro browser running.

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Landed in fx-team:
  https://hg.mozilla.org/integration/fx-team/rev/8e90d7b17aa5

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e90d7b17aa5

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

To land on Aurora plz

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/40473fad5b78

Comment 13

[Manuela Muntean [Away]]

Verified as fixed, for iteration #23, with latest Beta (28 beta 2) and latest Aurora 29.0a2 on Win 8.1 64-bit.