1164752 - Figure out how to include 360 secure browser in the startup migration

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[Hector Zhao [:hectorz]]

Beijing office ships a (bookmark only) migrator from 360 secure browser with its Firefox repack, make it available in the startup migration, pre-select it if 360 secure browser is detected as default browser. 

This was done by packaging this extension in distribution/bundles, now that distribution/bundles support is gone, we need another solution.

Comment 1

[Hector Zhao [:hectorz]]

Attachment: Patch for feedback

The initial conversion of the said extension to a mozilla-central patch.

(In reply to Hector Zhao [:hectorz] from bug 1144127 comment #39)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #35)
> > I am planning on proceeding with this with a B2G ifdef. Hector, ping me if
> > you think that the 360 browser import is important and let's decide whether
> > we should move that into the main tree or find some other solution that
> > doesn't open the door to hijacking.
> Hi Benjamin, yes, we still want it, as 360 secure browser accounts for ~1/4
> of startup migrations in Firefox China repack. I'll file a separate bug
> later, and convert the extension to a patch for feedback.
> 
> (In reply to Marco Bonardo [::mak] from comment #36)
> > Rather than a plain move of the 360 migrator in tree, it would be awesome to
> > have 360 added to the Firefox migration component itself, since we already
> > have the infrastructure, someone should just write a js migrator, it
> > shouldn't be too hard, we have migrators for Chrome, IE and Safari to look
> > at for examples.
> Hi Marco, our existing extension contains such a JS component, overlay for
> chrome://browser/content/migration/migration.xul to add the 360 secure
> browser option and the logic to (optionally) pre-select it, and override of
> chrome://browser/locale/migration/migration.properties for
> MigrationUtils.getLocalizedString to use.

Comment 2

[Benjamin Smedberg]

Comment on attachment 8605633 [details] [diff] [review]
Patch for feedback

I don't think I need to be in the critical path here. Mak are you good to review this?

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yes I can do that. I have some reviews backlog though that I hope to complete this week, so, I will try to handle this at the beginning of the next week.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8605633 [details] [diff] [review]
Patch for feedback

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

Is there a specific reason the migrator is for version 6? Will version 7 be completely different?
We usually handle different versions in the migrators themselves, rather than providing one migrator per version.
Unless there is a compelling reason, for now I'd just call it 360se and remove the 6 all around. (not in future we could still add versioning, if needed)

Tge patch looks mostly good, it is only importing bookmarks for now, but we can always expand it in future.
I have some questions/feedback:

::: browser/components/migration/360se6ProfileMigrator.js
@@ +5,5 @@
> +"use strict";
> +
> +let Cc = Components.classes;
> +let Ci = Components.interfaces;
> +let Cu = Components.utils;

nit, can use the short form 

const { classes: Cc, interfaces: Ci, results: Cr, utils: Cu } = Components;

@@ +39,5 @@
> +}
> +
> +function Bookmarks(aBookmarksFile) {
> +  this._file = aBookmarksFile;
> +  this._idToGuid = {};

nit: Might use a Map()

@@ +46,5 @@
> +  type: MigrationUtils.resourceTypes.BOOKMARKS,
> +
> +  migrate: function (aCallback) {
> +    return Task.spawn(function* () {
> +      let folderGuid = PlacesUtils.bookmarks.toolbarGuid;

So, looks like we are importing everything to the toolbar, does 360 have toolbar/menu concepts, or just the concept of more relevant and less relevant bookmarks?

@@ +53,5 @@
> +          yield MigrationUtils.createImportedBookmarksFolder("360se6", folderGuid);
> +      }
> +      this._idToGuid[0] = folderGuid;
> +
> +      let query = "SELECT id, parent_id, is_folder, title, url FROM tb_fav ORDER BY parent_id, pos;"

I suppose in 360 users can move bookmarks, that means this query might return a folder before its parent.
There's nothing stating folder 5 is inside folder 3, it might be the opposite and this would fail building the hierarchy.

You likely need a recursive query here, you can use WITH RECURSIVE for that, https://sqlite.org/lang_with.html there should be examples on how to deep visit a tree there. Otherwise I can help building a query for you.

@@ +68,5 @@
> +            url = row.getResultByName("url");
> +
> +        let parentGuid = this._idToGuid[parent_id];
> +        if (is_folder == 1) {
> +          let newFolderGuid = (yield PlacesUtils.bookmarks.insert({

Both folders and bookmarks creation might fail, for various reasons, like invalid URLs. Then this would not import anything after the failure point.
You likely want to try/catch around every creation, since folder creation can fail, before creating a bookmark you should check that parentGuid is valid.

@@ +70,5 @@
> +        let parentGuid = this._idToGuid[parent_id];
> +        if (is_folder == 1) {
> +          let newFolderGuid = (yield PlacesUtils.bookmarks.insert({
> +            parentGuid, type: PlacesUtils.bookmarks.TYPE_FOLDER, title: title
> +          })).guid;

please one property per line, for readability.
Also nit: just title instead of title: title

@@ +73,5 @@
> +            parentGuid, type: PlacesUtils.bookmarks.TYPE_FOLDER, title: title
> +          })).guid;
> +
> +          this._idToGuid[id] = newFolderGuid;
> +        } else if (is_folder == 0) {

Are there other possible values than 0 and 1? otherwhise this might just be a simple else

@@ +80,5 @@
> +          });
> +        }
> +      }
> +
> +      yield connection.close();

should likely be in a finally{} or we could leak the connection.

@@ +81,5 @@
> +        }
> +      }
> +
> +      yield connection.close();
> +      this._idToGuid = {};

Is there a reason this must be an object property rather than just a local map? if it would be local there should be no need to clear it.

@@ +95,5 @@
> +}
> +
> +Qihoo360se6ProfileMigrator.prototype = Object.create(MigratorPrototype);
> +
> +Qihoo360se6ProfileMigrator.prototype.getResources = function () {

This should get an aProfile argument, see below.

@@ +116,5 @@
> +    if (loginIniObj.NowLogin &&
> +        loginIniObj.NowLogin.IsAuto == 1 &&
> +        loginIniObj.NowLogin.UserMd5) {
> +      profile = loginIniObj.NowLogin.UserMd5;
> +    }

Are multiple profiles supported? Since in such a case you should rather implement the "sourceProfiles" property that returns an array of profile objects in the form { id, name } (id will then be passed to getResources, while name is used to ask the user which profile to import.
Note that we ask to the user only if there's more than 1 profile in the sourceProfiles array, so you could regardless move this code into the sourceProfiles getter, and eventually just return default/default. it should better separate the code.

@@ +131,5 @@
> +
> +  let resources = [];
> +  let file = profileDir.clone();
> +  file.append("360sefav.db");
> +  if (file.exists()) {

I'd prefer the approach taken by the IE migrator, where every migrator has an .exists property that states if the resource can be migrated, again it would allow to better separate the code.

Comment 5

[Hector Zhao [:hectorz]]

(In reply to Marco Bonardo [::mak] from comment #4)
> Comment on attachment 8605633 [details] [diff] [review]
> Patch for feedback

Thanks for all the feedback!

> 
> Review of attachment 8605633 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there a specific reason the migrator is for version 6? Will version 7 be
> completely different?
> We usually handle different versions in the migrators themselves, rather
> than providing one migrator per version.
> Unless there is a compelling reason, for now I'd just call it 360se and
> remove the 6 all around. (not in future we could still add versioning, if
> needed)

We first implemented the extension when v5 is the majority. When v6 introduced a seemingly major directory structure change, the fix was rushed out as a separate migrator w/o being merged with the v5 one.

The migrator in this patch actually works with both v6 and v7 (the current version). I'll drop the v6 reference, and make it compatible with v5 if possible.

> 
> @@ +46,5 @@
> > +  type: MigrationUtils.resourceTypes.BOOKMARKS,
> > +
> > +  migrate: function (aCallback) {
> > +    return Task.spawn(function* () {
> > +      let folderGuid = PlacesUtils.bookmarks.toolbarGuid;
> 
> So, looks like we are importing everything to the toolbar, does 360 have
> toolbar/menu concepts, or just the concept of more relevant and less
> relevant bookmarks?

The same set of bookmarks/folders are shown in both the menu and the toolbar.

> 
> @@ +53,5 @@
> > +          yield MigrationUtils.createImportedBookmarksFolder("360se6", folderGuid);
> > +      }
> > +      this._idToGuid[0] = folderGuid;
> > +
> > +      let query = "SELECT id, parent_id, is_folder, title, url FROM tb_fav ORDER BY parent_id, pos;"
> 
> I suppose in 360 users can move bookmarks, that means this query might
> return a folder before its parent.
> There's nothing stating folder 5 is inside folder 3, it might be the
> opposite and this would fail building the hierarchy.
> 
> You likely need a recursive query here, you can use WITH RECURSIVE for that,
> https://sqlite.org/lang_with.html there should be examples on how to deep
> visit a tree there. Otherwise I can help building a query for you.

I see, I'll try to figure this one out.

> 
> @@ +68,5 @@
> > +            url = row.getResultByName("url");
> > +
> > +        let parentGuid = this._idToGuid[parent_id];
> > +        if (is_folder == 1) {
> > +          let newFolderGuid = (yield PlacesUtils.bookmarks.insert({
> 
> Both folders and bookmarks creation might fail, for various reasons, like
> invalid URLs. Then this would not import anything after the failure point.
> You likely want to try/catch around every creation, since folder creation
> can fail, before creating a bookmark you should check that parentGuid is
> valid.

Should I just drop the bookmark if its parent folder is not created successfully?

> 
> @@ +81,5 @@
> > +        }
> > +      }
> > +
> > +      yield connection.close();
> > +      this._idToGuid = {};
> 
> Is there a reason this must be an object property rather than just a local
> map? if it would be local there should be no need to clear it.

No special reason, I'll switch to a local map.

> 
> @@ +116,5 @@
> > +    if (loginIniObj.NowLogin &&
> > +        loginIniObj.NowLogin.IsAuto == 1 &&
> > +        loginIniObj.NowLogin.UserMd5) {
> > +      profile = loginIniObj.NowLogin.UserMd5;
> > +    }
> 
> Are multiple profiles supported? Since in such a case you should rather
> implement the "sourceProfiles" property that returns an array of profile
> objects in the form { id, name } (id will then be passed to getResources,
> while name is used to ask the user which profile to import.
> Note that we ask to the user only if there's more than 1 profile in the
> sourceProfiles array, so you could regardless move this code into the
> sourceProfiles getter, and eventually just return default/default. it should
> better separate the code.

I also noticed this when I was preparing this patch, but I decided I can keep it for the initial feedback.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Hector Zhao [:hectorz] from comment #5)
> Should I just drop the bookmark if its parent folder is not created
> successfully?

I think you might import the bookmark into unfiled bookmarks if it has an invalid parent. Unfortunately that means you must handle the 2 different cases (initial import or not) to create the "From 360 Secure Browser" subfolder or merge the bookmarks directly into unfiled. You might have a lazy getter to create/return this emergency folder when needed.

Comment 7

[Hector Zhao [:hectorz]]

Attachment: MozReview Request: Bug 1164752 - include 360 secure browser in migration. r=mak

Bug 1164752: include 360 secure browser in migration

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://reviewboard.mozilla.org/r/10373/#review9205

Sorry, I still have some questions, mostly regarding the profiles code

::: browser/components/migration/360seProfileMigrator.js:55
(Diff revision 1)
> +  return hash;

nit: just return the row above? the hash temp var doesn't seem useful.

::: browser/components/migration/360seProfileMigrator.js:155
(Diff revision 1)
> +      this._defaultUserFolder = defaultUserFolder.leafName;

Does the default user always exist? Can the user remove it (for example in Firefox it's possible to remove the default profile)?

If the default profile can be removed and it is removed, this code would not find any profiles, even if they exist. In such a case it might be better to stop at the "users" folder here and just later try to check for the default folder and return it if it exists.

::: browser/components/migration/360seProfileMigrator.js:169
(Diff revision 1)
> +      return [];

return this.__sourceProfiles = [];

::: browser/components/migration/360seProfileMigrator.js:219
(Diff revision 1)
> +    }, this);

might use an arrow function to avoid passing "this".

::: browser/components/migration/360seProfileMigrator.js:92
(Diff revision 1)
> +";

please use a template string with ``, regardless the SQL engine doesn't really care about additional spaces.
The reason is that it makes much easier to copy paste the query for debug purposes.

::: browser/components/migration/360seProfileMigrator.js:88
(Diff revision 1)
> +     WHERE f.parent_id = b.id\

I prefer an explicit
JOIN bookmark AS b ON f.parent_id = b.id
instead of a cross join with a where

also, please specify the ASC ordering, even if it's the default.

::: browser/components/migration/360seProfileMigrator.js:49
(Diff revision 1)
> +                     stringStream.data = aStr ? aStr : "null";

please fix intentation, this should not be aligned with createInstance...

Looks like it would be simpler to provide "null" as a default value for the aStr argument.
But can this "null" case happen in reality or is it an error condition? Looks like we consider it a non important error and we still add the profile to the list, but doesn't that mean importing from that profile will fail later? Should we rather throw here, catch and continue in sourceProfiles so we only add valid profiles?

::: browser/components/migration/360seProfileMigrator.js:198
(Diff revision 1)
> +            loginIniObj[section].email == loginIniObj.NowLogin.email) {

in the previous if condition, you null-check loginIniObj.NowLogin, so I suppose it might be undefined.
In that case this loginIniObj.NowLogin.email would throw and we'd not add other profiles

::: browser/components/migration/360seProfileMigrator.js:214
(Diff revision 1)
> +    }

could you please add a comment explaining the logic here, for future reference.
Looks like you want the NowLogin profile at the top, if that profile is not logged in, you add the default profile on top of it, otherwise you add the default profile after any other profiles. Is this correct?

Here it is still relevant my question whether the default profile can be removed or not...

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8615915 [details]
MozReview Request: Bug 1164752 - include 360 secure browser in migration. r=mak

https://reviewboard.mozilla.org/r/10375/#review9215

Comment 10

[Hector Zhao [:hectorz]]

https://reviewboard.mozilla.org/r/10373/#review9365

> Does the default user always exist? Can the user remove it (for example in Firefox it's possible to remove the default profile)?
> 
> If the default profile can be removed and it is removed, this code would not find any profiles, even if they exist. In such a case it might be better to stop at the "users" folder here and just later try to check for the default folder and return it if it exists.

AFAIK, there's no UI like Firefox profile manager to remove the default user. I switched back to check the existence of the parent folder as advised.

> please fix intentation, this should not be aligned with createInstance...
> 
> Looks like it would be simpler to provide "null" as a default value for the aStr argument.
> But can this "null" case happen in reality or is it an error condition? Looks like we consider it a non important error and we still add the profile to the list, but doesn't that mean importing from that profile will fail later? Should we rather throw here, catch and continue in sourceProfiles so we only add valid profiles?

The existence of |.email| is checked before |getHash| is called with it, so "null" case here should never happen, I removed it.

Non-existing profiles will be filtered out at the end of |sourceProfiles| getter.

> could you please add a comment explaining the logic here, for future reference.
> Looks like you want the NowLogin profile at the top, if that profile is not logged in, you add the default profile on top of it, otherwise you add the default profile after any other profiles. Is this correct?
> 
> Here it is still relevant my question whether the default profile can be removed or not...

You're right about my intentions, comment added.

Comment 11

[Hector Zhao [:hectorz]]

Comment on attachment 8615915 [details]
MozReview Request: Bug 1164752 - include 360 secure browser in migration. r=mak

Bug 1164752: include 360 secure browser in migration

Comment 12

[Hector Zhao [:hectorz]]

Comment on attachment 8615915 [details]
MozReview Request: Bug 1164752 - include 360 secure browser in migration. r=mak

Bug 1164752: include 360 secure browser in migration

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8615915 [details]
MozReview Request: Bug 1164752 - include 360 secure browser in migration. r=mak

https://reviewboard.mozilla.org/r/10375/#review9397

please add 360seProfileMigrator to browser/installer/package-manifest.in close to the other Windows migrators (http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in#492)

I assume you tested this manually, since you are likely to notice problems better than me, due to language barriers. Regardless we need some QA support.
If you are willing to expand the migrator for other data or write unit tests (simulated, since we cannot install browsers on tinderboxes, examples are at http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/tests/unit/), feel free to file bugs for that and work on patches, I'll gladly review them.

Comment 14

[Hector Zhao [:hectorz]]

Comment on attachment 8615915 [details]
MozReview Request: Bug 1164752 - include 360 secure browser in migration. r=mak

Bug 1164752 - include 360 secure browser in migration. r=mak

Comment 15

[Hector Zhao [:hectorz]]

Comment on attachment 8615915 [details]
MozReview Request: Bug 1164752 - include 360 secure browser in migration. r=mak

https://reviewboard.mozilla.org/r/10375/#review9477

Ship It!

Comment 16

[Hector Zhao [:hectorz]]

(In reply to Hector Zhao [:hectorz] from comment #15)
> Comment on attachment 8615915 [details]
> MozReview Request: Bug 1164752 - include 360 secure browser in migration.
> r=mak
> 
> https://reviewboard.mozilla.org/r/10375/#review9477
> 
> Ship It!

Not sure how to directly carry over a r+ in MozReview.

Comment 17

[Hector Zhao [:hectorz]]

(In reply to Marco Bonardo [::mak] from comment #13)
> 
> I assume you tested this manually, since you are likely to notice problems
> better than me, due to language barriers. Regardless we need some QA support.
I'll ask QA in Beijing office test the try build when it's ready.

> If you are willing to expand the migrator for other data or write unit tests
> (simulated, since we cannot install browsers on tinderboxes, examples are at
> http://mxr.mozilla.org/mozilla-central/source/browser/components/migration/
> tests/unit/), feel free to file bugs for that and work on patches, I'll
> gladly review them.
I'll try and see if I can create some tests, thanks!

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 8615915 [details]
MozReview Request: Bug 1164752 - include 360 secure browser in migration. r=mak

https://reviewboard.mozilla.org/r/10375/#review9479

Ship It!

Comment 19

[Hector Zhao [:hectorz]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=adbc09e5cc76

(In reply to Hector Zhao [:hectorz] from comment #17)
> (In reply to Marco Bonardo [::mak] from comment #13)
> > 
> > I assume you tested this manually, since you are likely to notice problems
> > better than me, due to language barriers. Regardless we need some QA support.
> I'll ask QA in Beijing office test the try build when it's ready.

Our QA tested the try build and the integrated 360se migrator works as expected.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/eeeaa094d312

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/eeeaa094d312

Comment 22

[Francesco Lodolo [:flod]]

I confess that I've never heard of this "360 Secure Browser", so I tried searching for it when I found the strings in m-c. 

I find more results with "360 Safe Browser" than "360 Secure Browser", is this the same product? Is there an official English name for it?
http://www.360.cn/about/englishversion.html

Comment 23

[Hector Zhao [:hectorz]]

(In reply to Francesco Lodolo [:flod] from comment #22)
> 
> I find more results with "360 Safe Browser" than "360 Secure Browser", is
> this the same product? Is there an official English name for it?
> http://www.360.cn/about/englishversion.html

I believe it's the same product, both of these two translations are used, I don't really remember why I choosed this particular one.

I searched these two translations with "site:360.cn", it appears "360 Secure Browser" is used only once, while "360 Safe Browser" multiple times. I'll prepare a patch to change the strings to "360 Safe Browser". Thanks for bring this up.

Comment 24

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Hector Zhao [:hectorz] from comment #19)
> Our QA tested the try build and the integrated 360se migrator works as
> expected.

Removing qe-verify+ flag since this was already verified by a QA resource in the Beijing office.