remove toolkit/identity/

RESOLVED FIXED in Firefox 54

Status

()

Toolkit
General
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: aryx, Assigned: stefanh)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Implemented in bug 762993, looks like only used on B2G. And B2G gets removed from mozilla-central in bug 1306391 .
(Assignee)

Comment 1

11 months ago
Created attachment 8811868 [details] [diff] [review]
Remove it

Builds fine locally. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=be19a6fe5a134a3738280348bdb6b2c65529860d
Assignee: nobody → stefanh
Status: NEW → ASSIGNED
Attachment #8811868 - Flags: review?(dolske)
(Assignee)

Comment 2

11 months ago
Comment on attachment 8811868 [details] [diff] [review]
Remove it

Whoops, I never ran the installer...
Attachment #8811868 - Flags: review?(dolske)
(Assignee)

Comment 3

11 months ago
Created attachment 8811897 [details] [diff] [review]
Correct version

I forgot to remove identity.xpt from installer/package-manifest.in... New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8c394bb17fb53dc5c63ee2f18849c235dc9e3e5
Attachment #8811868 - Attachment is obsolete: true
Attachment #8811897 - Flags: review?(dolske)

Comment 4

11 months ago
Comment on attachment 8811868 [details] [diff] [review]
Remove it

(I assume you didn't mean to obsolete this part, which is doing the bulk of the removal :).
Attachment #8811868 - Attachment is obsolete: false

Comment 5

11 months ago
Comment on attachment 8811868 [details] [diff] [review]
Remove it

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

Hmm, this seems to be missing a few files. I don't see a deletion for:

IdentityUtils.jsm
jwcrypto.jsm (but this is still used in services/fxaccounts, so it and associated tests should probably be moved)
RelyingParty.jsm


r- for a few nits and to catch those missing files, but otherwise on the right track! Thanks for doing this.

::: toolkit/identity/FirefoxAccounts.jsm
@@ -1,1 @@
> -/* This Source Code Form is subject to the terms of the Mozilla Public

There's a reference to this file in ~/.eslintignore that can be removed too

::: toolkit/identity/LogUtils.jsm
@@ -1,1 @@
> -/* -*- js-indent-level: 2; indent-tabs-mode: nil -*- */

tools/lint/eslint/modules.json contains a reference to LogUtils.jsm that can be removed.

::: toolkit/identity/MinimalIdentity.jsm
@@ -1,1 @@
> -/* -*- js-indent-level: 2; indent-tabs-mode: nil -*- */

tools/lint/eslint/modules.json contains a reference to MinimalIdentity.jsm that can be removed.

::: toolkit/identity/tests/unit/test_jwcrypto.js
@@ -1,1 @@
> -/* Any copyright is dedicated to the Public Domain.

jwcrypto.jsm is still used a wee bit in /services -- http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/services/fxaccounts/FxAccounts.jsm#28-29

So it and its tests should stay alive. As noted in the overview, probably best to just move this over to /services.
Attachment #8811868 - Flags: review-

Comment 6

11 months ago
Comment on attachment 8811897 [details] [diff] [review]
Correct version

Looks fine. Can you just roll this into the previous patch with the deletions?

Although I'd suggest having a separate patch, taking care of moving jwcrypto.jsm + tests, so it's clear in Hg history that there's one changeset moving a couple files, and another changeset nuking everything else.

(I'm assuming the jwcrypto move is trivial, if it's not I could be convinced to leave it in place for now.)
Attachment #8811897 - Flags: review?(dolske)
Blocks: 1244950
(Assignee)

Comment 7

11 months ago
Hmm, I don't know this code at all, but I spent a few hours looking at this and there are more dependencies than I first noticed:

http://searchfox.org/mozilla-central/rev/d98418da69edeb1f2f8e6f3840157fae1512f89b/toolkit/identity/jwcrypto.jsm#17 for example. And looking at test_jwcrypto.js, it needs more from toolkit/identity: http://searchfox.org/mozilla-central/rev/d98418da69edeb1f2f8e6f3840157fae1512f89b/toolkit/identity/tests/unit/test_jwcrypto.js#8

And then looking at /toolkit/identity/Identity.jsm (needed by test_jwcrypto.js), it looks like it uses IdentityStore.jsm, RelyingParty.jsm and IdentityProvider.jsm.
(Assignee)

Updated

11 months ago
Status: ASSIGNED → NEW
(Assignee)

Comment 8

11 months ago
Is the jwcrypto.js code in FxAccounts.jsm still used?

Comment 9

11 months ago
Oh, derp, I didn't look to see if jwcrypto depended on stuff. I assumed it was independent. And the try failures from the comment 3 link further indicate there's a dependency. :)

So I think this means, in addition to keeping the jwcrypto bits:
1) Keep IdentityCryptoService stuff
2) Keep LogUtils.jsm (or nuke it and change its usage to something else)
3) Keep test_crypto_service.js and any needed bits from head_identity.js

I didn't look at the tests in detail, but for at least the one example linked in comment 7 that Cu.import is actually unused.

Mark, any thoughts here? I assume FxAccounts.jsm still needs the jwcrypto bits. Does moving jwcrypto + IdentityCryptoService over to /services/fxaccounts/ make sense to you too? Have any cycles to help move the code?

Stefan, if you're not able to move the code yourself, I'd still take a patch that nukes almost everything and leaves needed things in place. Although if untangling some of the dependencies is still an issue, might be easier to see if Mark can just help more the still-needed code first. Then /toolkit/identity would just be a simple deletion.
Oops, meant to actually NI Mark. :) See last comment.
Flags: needinfo?(markh)
(Assignee)

Comment 11

11 months ago
Let's wait for Mark ;-)

But it looks like getAssertionFromCert and getKeypairAndCertificate in FxAccounts.jsm needs jwcrypto.js. And from what I can see getKeypairAndCertificate is used in services/fxaccounts/tests/xpcshell/test_accounts.js
(In reply to Justin Dolske [:Dolske] from comment #9)
> Mark, any thoughts here? I assume FxAccounts.jsm still needs the jwcrypto
> bits. Does moving jwcrypto + IdentityCryptoService over to
> /services/fxaccounts/ make sense to you too?

Yep - or into the existing services/crypto dir.

> Have any cycles to help move the code?

Not before Hawaii :( But I think that should be relatively easy - best I can tell, there's exactly 1 import of jwcrypto in all of sync/fxa.
Flags: needinfo?(markh)
(Assignee)

Comment 13

11 months ago
Created attachment 8815485 [details] [diff] [review]
Removal of everything in toolkit/identity

Just for reference, here's a complete removal of toolkit/identity where also the dependencies of jwcrypto.js in FxAccounts.jsm are removed. I haven't disabled services/fxaccounts/tests/xpcshell/test_accounts.js, though.
(In reply to Stefan [:stefanh] from comment #13)
> Just for reference, here's a complete removal of toolkit/identity where also
> the dependencies of jwcrypto.js in FxAccounts.jsm are removed.

I'm a little confused. What's the purpose of this?

> I haven't
> disabled services/fxaccounts/tests/xpcshell/test_accounts.js, though.

We can't disable that test, nor can we remove jwcrypto from FxAccounts.jsm.
(Assignee)

Comment 15

11 months ago
(In reply to Mark Hammond [:markh] from comment #14)
> (In reply to Stefan [:stefanh] from comment #13)
> > Just for reference, here's a complete removal of toolkit/identity where also
> > the dependencies of jwcrypto.js in FxAccounts.jsm are removed.
> 
> I'm a little confused. What's the purpose of this?

I just wanted to put it somewhere and use bits from it. I'll take a look at the move, will hopefully have something that works next week.
(Assignee)

Comment 16

11 months ago
Created attachment 8817648 [details] [diff] [review]
Part1: Remove unneeded files/bits

I took a slightly different route when it comes to splitting this in 2 parts, mainly because I've been struggling a bit with the move. This part will remove files/bits not needed, but keep the needed files in toolkit/identity. Everything should work fine :-)
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce66066220a12f5fe56b43def78c88bf6159c714.

The above push is nearly identical to an older push (where I forgot to remove the const PREF_DEBUG = "toolkit.identity.debug" in LogUtils.jsm) which looked good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=907278ec8599803a7cddf7fe8e90142f0e39dd21

Some comments:
1) I removed the toolkit.identity.debug pref and edited LogUtils.jsm a bit. I don't think a pref for this is needed, since the LogUtils.jsm logging is only used in tests.
2) I kept head_identity.js for now, but it will go in the next part.
3) Turns out that the xpcshell tests have been b2g-only, but the 2 tests works fine on almost all other platforms (they fail on Android, so I turned them off for Android).
Attachment #8811868 - Attachment is obsolete: true
Attachment #8811897 - Attachment is obsolete: true
Attachment #8815485 - Attachment is obsolete: true
Attachment #8817648 - Flags: review?(dolske)
(Assignee)

Comment 17

11 months ago
Created attachment 8817667 [details] [diff] [review]
part2: Move used files/bits to services/crypto

Here's part2. nsIIdentityCryptoService is now in services-crypto-component, so I updated the CID.
Attachment #8817667 - Flags: review?(dolske)
(Assignee)

Comment 18

11 months ago
Try push (Part1 + Part2): https://treeherder.mozilla.org/#/jobs?repo=try&revision=288c74777c58be4b1620a1397d5019b4e579d410
(Assignee)

Comment 19

9 months ago
Patches doesn't apply anymore. But if this is still interesting, I can update them.
Status: NEW → ASSIGNED
Flags: needinfo?(dolske)
Blocks: 1316187
(In reply to Stefan [:stefanh] from comment #19)
> Patches doesn't apply anymore. But if this is still interesting, I can
> update them.

Yes please. Even just removing low-hanging fruit would be great.
(Assignee)

Comment 21

9 months ago
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #20)
> (In reply to Stefan [:stefanh] from comment #19)
> > Patches doesn't apply anymore. But if this is still interesting, I can
> > update them.
> 
> Yes please. Even just removing low-hanging fruit would be great.

OK, I'll try to have something up for review in a few days.
Flags: needinfo?(dolske)
(Assignee)

Updated

9 months ago
Attachment #8817648 - Flags: review?(dolske)
(Assignee)

Updated

9 months ago
Attachment #8817667 - Flags: review?(dolske)
(Assignee)

Comment 22

9 months ago
Created attachment 8829271 [details] [diff] [review]
Part1: Remove unneeded files/bits

This is the removal part, next part will move the needed files to services/crypto (and remove the remaining files in toolkit/identity). See also comment #16. Results from try push is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb5f783b4cb3fc3e8448435a92f607d855f54791
Attachment #8817648 - Attachment is obsolete: true
Attachment #8817667 - Attachment is obsolete: true
Attachment #8829271 - Flags: review?(MattN+bmo)
(Assignee)

Comment 23

9 months ago
Created attachment 8829272 [details] [diff] [review]
Part2: Move stuff to services/crypto and remove remaining files in toolkit/identity

This is part2 which will move stuff to services/cryptoand nuke the remaining files in toolkit/identity. See also comment #17. Try push (Part1 + Part2): Try push, part1 + part 2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9871cdcc6a1468195292b762caf19fb56b356c72
Attachment #8829272 - Flags: review?(MattN+bmo)
Comment on attachment 8829271 [details] [diff] [review]
Part1: Remove unneeded files/bits

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

Thanks Stefan!

::: toolkit/identity/LogUtils.jsm
@@ -18,5 @@
>  Cu.import("resource://gre/modules/Services.jsm");
>  
> -function IdentityLogger() {
> -  Services.prefs.addObserver(PREF_DEBUG, this, false);
> -  this._debug = Services.prefs.getBoolPref(PREF_DEBUG);

This logging needs to stay behind a pref as we don't want to spew non-error messages in regular builds. Can you revert the changes to this file but switch the pref to services.sync.log.cryptoDebug for now? Then file a follow-up in the services product to remove this file and replace the logging in jwcrypto.jsm the Log.jsm approach used in the rest of the related services code?

@@ -68,5 @@
>  
>    /**
>     * log() - utility function to print a list of arbitrary things
>     *
> -   * Enable with about:config pref toolkit.identity.debug

Don't forget to update this comment with services.sync.log.cryptoDebug
Attachment #8829271 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8829272 [details] [diff] [review]
Part2: Move stuff to services/crypto and remove remaining files in toolkit/identity

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

r=me if we leave the CID alone.

Thanks

Perhaps you want to take bug 1244950 or removing LogUtils.jsm as a next bug?

::: services/crypto/component/IdentityCryptoService.cpp
@@ +543,5 @@
>    { nullptr }
>  };
>  
>  const mozilla::Module::ContractIDEntry kContracts[] = {
> +  { "@mozilla.org/services-crypto/identity;1", &kNS_IDENTITYCRYPTOSERVICE_CID },

"@mozilla.org/services-crypto/identity;1" doesn't seem like a good name to me. Usually services end with "service" and the suffix usually describes what it's about. In this case I think "identity" is confusing. Since the file is still IdentityCryptoService.cpp I think the old contract name is fine. I also wonder if you would need to request a CLOBBER if you change this? I think it's easiest to just leave this alone throughout the patch.
Attachment #8829272 - Flags: review?(MattN+bmo) → review+
(Assignee)

Comment 26

9 months ago
Thanks, I'll make all your suggested/required changes and push this in the next day or so.

> Perhaps you want to take bug 1244950 or removing LogUtils.jsm as a next bug?

Let me see what I can do, I'll start with filing a bug for the LogUtils.jsm removal ;-)

Comment 27

9 months ago
Pushed by stefanh@inbox.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f4f047cfc54
Remove toolkit/identity, part1: Remove unneeded files/bits. r=MattN.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bab9319950e
Remove toolkit/identity, part2: Move used files/bits to services/crypto and remove remaining files in toolkit/identity. r=MattN.
(Assignee)

Updated

9 months ago
Blocks: 1333485
(Assignee)

Comment 28

9 months ago
(In reply to Stefan [:stefanh] from comment #26)
> Let me see what I can do, I'll start with filing a bug for the LogUtils.jsm
> removal ;-)
Filed bug 1333485.

Comment 29

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7f4f047cfc54
https://hg.mozilla.org/mozilla-central/rev/1bab9319950e
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

9 months ago
Depends on: 1333692
Depends on: 1351659
Blocks: 1369194
No longer blocks: 1369194
You need to log in before you can comment on or make changes to this bug.