Closed Bug 1178166 Opened 4 years ago Closed 4 years ago

Handle gbk encoded login.ini from 360 secure browser

Categories

(Firefox :: Migration, defect)

41 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox41 + verified
firefox42 --- verified

People

(Reporter: hectorz, Assigned: hectorz)

References

Details

Attachments

(1 file, 1 obsolete file)

QA from Beijing office found out the profile selection page will not be shown if there's Chinese characters in 360 secure browser's login.ini. It is encoded in gbk, while nsIINIParser assumes utf-8.
Blocks: 1164752
Version: Trunk → 41 Branch
Attached patch Patch for feedback (obsolete) — Splinter Review
This patch convert the gbk encoded login.ini to an utf-8 encoded file in TmpD, and create the nsIINIParser with the temp file.

It works for me locally, but I'm not sure whether this is the right approach.
Attachment #8627092 - Flags: feedback?(mak77)
Comment on attachment 8627092 [details] [diff] [review]
Patch for feedback

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

So there are various possibilities:

1. Update nsINIParser so it can handle any charset. This is the more future-proof option, basically change createINIParser to take an optional charset... On the other side this might be a lot of work. Moreover I can't figure why we have @mozilla.org/xpcom/ini-parser-factory;1 and @mozilla.org/xpcom/ini-processor-factory;1, one in CPP and one in JS, they seem to do similar things. Looks like Dolske wrote the js version so he might know the difference and if one of the two is deprecated, or even just why we have 2 implementations.
2. Your workaround, but moved to an helper method, for readability. We should also try to remove the temp file once done, so we don't even risk to expose some user data outside of the profile folder.
3. Your workaround but using NetUtil.asyncCopy. The problem here is that sourceProfiles is synchronous, this this would again require a lot of changes for a limited use-case.

Considered this is a limited use-case, both 1 and 3 seem to require too much effort for the benefit.
1 has the most benefit (future support) but having to update 2 codebases, (js and cpp) is a no-go imo.
I'd say to go for 2, but just in case I'm also needinfoing Dolske to understand if he has an idea regarding the current status of INIParser and why we apparently have multiple implementations. (we also have a java implementation obviously!)
Attachment #8627092 - Flags: feedback?(mak77) → feedback+
Flags: needinfo?(dolske)
[Tracking Requested - why for this release]: first version with the new 360 migrator
(In reply to Marco Bonardo [::mak] from comment #2)

> Moreover I can't
> figure why we have @mozilla.org/xpcom/ini-parser-factory;1 and
> @mozilla.org/xpcom/ini-processor-factory;1, one in CPP and one in JS, they
> seem to do similar things. Looks like Dolske wrote the js version so he
> might know the difference and if one of the two is deprecated, or even just
> why we have 2 implementations.

The C++ one can only read INI files, and the JS one can only write files. This got implemented in bug 541594.


> Considered this is a limited use-case, both 1 and 3 seem to require too much
> effort for the benefit.
> 1 has the most benefit (future support) but having to update 2 codebases,
> (js and cpp) is a no-go imo.
> I'd say to go for 2

I agree, seems simplest to just do what the workaround here does.

How do you see this as needing to update 2 code bases? Are you thinking the helper should live in the INI reader/writer?

I think it would be fine to leave this code in 360seProfileMigrator.js (although as an explicit helper function would be nice). If you really want it in to live somewhere else, I'd treat it as a generic "make a copy of this file while converting the charset" helper, and stuff it in some JSM with other file i/o helpers (since it's not really INI specific).
Flags: needinfo?(dolske)
(In reply to Justin Dolske [:Dolske] from comment #4)
> How do you see this as needing to update 2 code bases? Are you thinking the
> helper should live in the INI reader/writer?

This was about implmenting option 1, thus actually fixing the underlying support in the iniparser. And I said 2 codebases cause I thought both cpp and js could read ini files.

I'm also fine keeping this in an helper in the migrator itself. Good, we agree!
:mak and :Dolske, thanks for the feedback! I'll update the patch later.
Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r?mak
Attachment #8629777 - Flags: review?(mak77)
Attachment #8627092 - Attachment is obsolete: true
Attachment #8629777 - Flags: review?(mak77) → review+
Comment on attachment 8629777 [details]
MozReview Request: Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak

https://reviewboard.mozilla.org/r/12641/#review11125

LGTM

::: browser/components/migration/360seProfileMigrator.js:226
(Diff revision 1)
> +      loginIniInUtf8.remove(false);

Windows is famous for long lasting file locks, let's wrap this remove with a try/catch, it's not a good enough reason to stop the migration.

::: browser/components/migration/360seProfileMigrator.js:28
(Diff revision 1)
> +  let copyInUtf8 = FileUtils.getFile(

let tempUTF8File =

::: browser/components/migration/360seProfileMigrator.js:21
(Diff revision 1)
> +function createCopyInUtf8(file, charset, tempFileName) {

what about naming this "copyToUTF8TempFile"
Also, we can reuse the original file leafName, or generate a random name, we don't need it to take a tempFileName.

::: browser/components/migration/360seProfileMigrator.js:23
(Diff revision 1)
> +                    createInstance(Ci.nsIFileInputStream);

<p>nit: please align as<br />
Cc[...<br />
  .createInstance(...</p>
Comment on attachment 8629777 [details]
MozReview Request: Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak

Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak
Attachment #8629777 - Attachment description: MozReview Request: Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r?mak → MozReview Request: Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak
Attachment #8629777 - Flags: review+ → review?(mak77)
Comment on attachment 8629777 [details]
MozReview Request: Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak

https://reviewboard.mozilla.org/r/12641/#review11277

::: browser/components/migration/360seProfileMigrator.js:32
(Diff revision 2)
> +  tempUTF8File.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0o600);

Actually, since you use createUnique, you don't need the rand part, it will already take care of creatina a unique leaf name. let's just use file.leafName then.
Attachment #8629777 - Flags: review?(mak77) → review+
Assignee: nobody → bzhao
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/12641/#review11277

> Actually, since you use createUnique, you don't need the rand part, it will already take care of creatina a unique leaf name. let's just use file.leafName then.

This is me trying to replicate the logic here https://hg.mozilla.org/mozilla-central/file/e271ef4c49ae/xpcom/io/nsAnonymousTemporaryFile.cpp#l134 , would it be okay if I keep it like this?
(In reply to Hector Zhao [:hectorz] from comment #11)
> This is me trying to replicate the logic here
> https://hg.mozilla.org/mozilla-central/file/e271ef4c49ae/xpcom/io/
> nsAnonymousTemporaryFile.cpp#l134 , would it be okay if I keep it like this?

ok, I see the reasoning, please just add a comment that we use random to reduce the likelyhood of hitting a name collision in createUnique. Otherwise it's unclear why we do that.
Comment on attachment 8629777 [details]
MozReview Request: Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak

Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak
Attachment #8629777 - Flags: review+ → review?(mak77)
Attachment #8629777 - Flags: review?(mak77) → review+
Comment on attachment 8629777 [details]
MozReview Request: Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak

https://reviewboard.mozilla.org/r/12641/#review11285

Ship It!

::: browser/components/migration/360seProfileMigrator.js:31
(Diff revision 3)
> +   */

please use single line comments instead of a multi line comment, since those allow to comment out parts of code more easily for debugging purposes.
Attachment #8629777 - Flags: review+ → review?(mak77)
Comment on attachment 8629777 [details]
MozReview Request: Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak

Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak
Comment on attachment 8629777 [details]
MozReview Request: Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak

https://reviewboard.mozilla.org/r/12641/#review11287

Ship It!
Attachment #8629777 - Flags: review?(mak77) → review+
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c88d16a3c469

(In reply to Marco Bonardo [::mak] from comment #16)
> 
> Ship It!

Thanks!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1f20004e8d2d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Adding a tracking flag for Firefox 41. We should certainly make it very easy for Firefox users to import settings/bookmarks etc from other browsers.
Comment on attachment 8629777 [details]
MozReview Request: Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak

Approval Request Comment
[Feature/regressing bug #]: new 360 Secure Browser migrator
[User impact if declined]: the migrator doesn't work in some cases
[Describe test coverage new/current, TreeHerder]: Nightly
[Risks and why]: low risk, just touches the migrator itself
[String/UUID change made/needed]: none
Attachment #8629777 - Flags: approval-mozilla-aurora?
Comment on attachment 8629777 [details]
MozReview Request: Bug 1178166 - Handle gbk encoded login.ini from 360 secure browser. r=mak

Low risk, just touches the migrator itself, has been in m-c for two days.
Attachment #8629777 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
It works fine on latest Nightly 42.0a1 and developer edition 41.0a2.
p.s. Is there any flag need to be changed?
(In reply to Bingqing Li from comment #24)
> p.s. Is there any flag need to be changed?
Since this bug has been verified and I don't really know about the flags, I'd like to know is there any flag need to be changed after the verification.
Just changed RESOLVED to VERIFIED, and change the fixed to verified in the status-version flags
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.