297915 - Migration doesn't open from clean launch if the name of the default browser ends with '.EXE' (instead of '.exe')

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[Tracy Walker [:tracy]]

Seen on Windows Deer Park 2005-06-16-07-trunk
This has regressed since yesterdays smoketested build - 2005-06-15-07-trunk

-delete or rename Firefox profile folder
-Launch Deer Park

tested results:
Deer Park launches without going through Migration dialogs

expected results:
Migration dialog opens

notes: 
-Import from File | Import works
-migration works as expected on Mac OSX

Comment 1

[Tracy Walker [:tracy]]

Migration works on linux Deer Park 2005-06-16-05-trunk

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Bug 250255 is the likely candidate for breaking this.

Range:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-06-15+07%3A00%3A00&maxdate=2005-06-16+07%3A00%3A00&cvsroot=%2Fcvsroot

Comment 3

[chris hofmann]

need to sort this out for 1.1 a2

Comment 4

[Jungshik Shin]

I can't reproduce the bug  with the latest nightly build. What exactly did you
delete or rename?  Deleting ~/Mozilla/Firefox/Profiles or
Mozilla/Firefox/Profiles/<all profile folders>, firefox refuses to  start saying
firefox is still running although there's no trace of its running in the task
manager. Deleting Mozilla/Firefox and starting the latest nightly, I was
prompted to pick a browser to migrate from. 

Btw, what was your default browser when you experienced this problem?

Comment 5

[Tracy Walker [:tracy]]

hmmm, thought I commented here.  anyway, key to reproduce this is to set default
to Moz suite.  if it's set as IE of Firefox it will not appear.

Comment 6

[Marcia Knous [:marcia]]

Tracy: We need clarification here as the exact steps you followed and what was
set as the default browser.

(In reply to comment #5)
> hmmm, thought I commented here.  anyway, key to reproduce this is to set default
> to Moz suite.  if it's set as IE of Firefox it will not appear.

Comment 7

[Tracy Walker [:tracy]]

oi, comment 5 is not meant for this bug. sorry.

Migration is supposed to fire if we don't have a Firefox profile. so:

-delete or rename C:\Documents and Setting\*user*\Application Data\Mozilla\Firefox
-Then launch Deer Park.

The Migration dialog (import wizard) should open, but it doesn't launch.

performing the same intialization then launch any Fx branch release launches
Migration as expected. So I know this isn't something machine/setup related. 
Plus I was seeing this on MoFo machine as well as remotely. 

I double checked the regression window as initially reported: the 0615 build
works, all builds 0616 and after don't work.

Comment 8

[Jungshik Shin]

Why is comment #5 not for this bug? For sure, it is. If you had told us that
this problem only happened with Mozilla suite as the default browser in comment
#0, it would have saved me some time. Anyway, I can reproduce the bug with the
default browser set to mozilla suite and I've just come up with a patch. I was
confused about |Find| API for our string class. (for 'UTF-16' strings, the
second argument is not for specifying 'case-sensitivity'). Sorry for the regression.

Comment 9

[Jungshik Shin]

Attachment: patch

I'm not using FindInReadable with 'case-insenstive' string comparator because
it may be too expensive here. On the other hand, I have to convert back and
forth between UTF-8 and UTF-16 so that overall I may be saving very little in
time if any (at the cost of more verbose code and a larger code size). If you
think FindInReadable is better, let me know and I'll make a new patch.

Comment 10

[Jungshik Shin]

oops. I've already gotten rid of two assertions I added for debugging in my tree.

Comment 11

[Tracy Walker [:tracy]]

heh, comment #5 was coincidental. But I'm glad it helped you uncover the
problem. I didn't realize setting Mozilla as default was causing this.  I was
speaking in comment #5 about bug 262326.  It is also about an issue with the
migration dialog on windows that is caused by setting Mozilla suite as default.
 Perhaps you could take a look at it?  Or maybe your patch here will also fix
that bug?  thanks jshin.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 187589 [details] [diff] [review]
patch 

+  NS_ASSERTION(0, "nsPM::Migrate");

I know you are removing it, but use NS_ERROR for assertions with a constant :)

So, I don't think this code is verys performance-sensitive. I'd prefer it if
the code didn't convert between UTF-8 and UTF-16 constantly.

Comment 13

[Darin Fisher]

Comment on attachment 187589 [details] [diff] [review]
patch 

make biesi happy, then re-request sr ;-)

Comment 14

[Jungshik Shin]

Attachment: patch with FindInReadable

Using FindInReadable and iterators makes a bit cryptic part of the code  easier
to understand (the part cbie asked me about in another bug :-))

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 187931 [details] [diff] [review]
patch with FindInReadable 

the code is now a bit different from the old code, does that matter? I actually
think it does if the default browser had any arguments - the old code only
included the part of the string up to the .exe, you are now including
everything.

(I don't get the point of the ':' check. is it for handling a leading quote? or
something else?)

Comment 16

[Jungshik Shin]

(In reply to comment #15)

> included the part of the string up to the .exe, you are now including
> everything.

 No, it doesn't. Note that FindInReadable changes two string iterators to point
to the beginning and the end of a pattern actually found. That is, string
iterators are in/out parameters.
 
> (I don't get the point of the ':' check. is it for handling a leading quote? 

I don't know either. I'm just preserving the current behavior. Perhaps, to
handle a difference(?) between c:\abc\def\moz.exe and \\host\abc\def\moz.exe ??

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 187931 [details] [diff] [review]
patch with FindInReadable 

oh. it was FindCharInReadable that only modified one of them. sorry about that.

Comment 18

[Jungshik Shin]

Comment on attachment 187931 [details] [diff] [review]
patch with FindInReadable 

thanks. btw, I think you're right that checking ':' is to remove the leading
quotation mark.

Comment 19

[Darin Fisher]

Comment on attachment 187931 [details] [diff] [review]
patch with FindInReadable 

>Index: browser/components/migration/src/nsProfileMigrator.cpp

>-  PRInt32 lastIndex = value.Find(NS_LITERAL_STRING(".exe"), PR_TRUE);
>-  if (lastIndex == kNotFound)
>+  nsAString::const_iterator start, end;
>+  value.BeginReading(start);
>+  value.EndReading(end);
>+  nsAString::const_iterator tmp = start;
>+
>+  if (!FindInReadable(NS_LITERAL_STRING(".exe"), tmp, end, 
>+                      nsCaseInsensitiveStringComparator()))
>     return NS_ERROR_FAILURE;
> 
>-  int index = (value.CharAt(1) == ':') ? 0 : 1;
>-  nsDependentSubstring filePath(value, index, lastIndex + 4 - index); 
>+  if (value.CharAt(1) != ':')
>+    ++start;
>+
>+  nsDependentSubstring filePath(start, end); 

I admit that this change confuses me.  I understand that the existing
code computes |index| badly, but I don't understand why you trim ".exe"
from the end of the string now.  The old code did not trim it.	Moreover,
what if my browser executable is named "foo.exe.com" for some odd reason?
I'm not sure the code you're replacing makes much sense, and I'm not sure
the new code makes sense either.  Am I missing something?

Comment 20

[Jungshik Shin]

(In reply to comment #19)
> (From update of attachment 187931 [details] [diff] [review] [edit])

> >+  nsAString::const_iterator start, end;

> >+  if (!FindInReadable(NS_LITERAL_STRING(".exe"), tmp, end, 
> >+                      nsCaseInsensitiveStringComparator()))
> >     return NS_ERROR_FAILURE;
[snip]

> >+  nsDependentSubstring filePath(start, end); 
> 
> I admit that this change confuses me.  I understand that the existing
> code computes |index| badly, but I don't understand why you trim ".exe"
> from the end of the string now.  The old code did not trim it.

Unless I misread the comment for FindInReadable
(http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/nsReadableUtils.h#283),
|end| should points to the end of the match (i.e. the second 'e' in '.exe') when
there's a match. So, |nsDependentSubstring(start, end)| does not trim '.exe'. 

> Moreover, what if my browser executable is named "foo.exe.com" for some 
> odd reason?


> I'm not sure the code you're replacing makes much sense, and I'm not sure
> the new code makes sense either.  Am I missing something?

As far as I can tell, what both of them do is :

Given |"C:\full\path with spaces\mozilla.exe" -Options| or
|C:\full\path\msie.exe /Options|, get |C:\full\path with spaces\mozilla.exe| 
or |C:\full\path\msie.exe|.  

What to do with an odd case like '....browser.exe.com'? Hmm.... There are other
problematic cases like '....browser.exe.exe' or 'c:\....\brow.exe.blah.exe' 

We can make this more robust than now by checking what 'exe' is followed by (or
whether it's followed by anything), but I'm not sure  it is worth extra code. 

Comment 21

[Darin Fisher]

Comment on attachment 187931 [details] [diff] [review]
patch with FindInReadable 

ok, right... this works.  i think we can punt on the foo.exe.exe problem for
now.

Comment 22

[Jungshik Shin]

Comment on attachment 187931 [details] [diff] [review]
patch with FindInReadable 

asking for a 1.0.5
This is to fix a regression introduced recently and should be fixed asap.

Comment 23

[chris hofmann]

Comment on attachment 187931 [details] [diff] [review]
patch with FindInReadable 

a=chofmann for 1.0.5

Comment 24

[Jungshik Shin]

fix checked into the trunk