Closed Bug 618195 Opened 14 years ago Closed 13 years ago

Build services-crypto (or just nsSyncJPAKE) unconditionally as part of tier_platform

Categories

(Firefox :: Sync, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: philikon, Assigned: glandium)

References

Details

(Whiteboard: [qa-])

Attachments

(4 files, 2 obsolete files)

      No description provided.
We're going to need to resolve this before Firefox on Xulrunner works with the patch for Bug 601645.
I wrote yesterday on irc that I would take care of this followup today. There's also a need to move the static component part in a separate directory and have that in tier_platform. The remainder needs to be built as part of the browser build.
Assignee: nobody → mh+mozilla
This part just builds the static component unconditionally.
Attachment #496794 - Flags: review?
Attachment #496794 - Flags: review? → review?(mconnor)
services/crypto actually contains two things:
- the crypto js module
- the crypto (j-pake) c++ component

The former needs to be shipped with the browser, while the latter needs to be shipped with the platform. This means they should be separated from each other.

This patch moves the c++ component into services/crypto/component, and makes only that directory part of tier_platform. That directory is not defined as a SUBDIR of services/crypto, so that it doesn't get built when going into services/crypto, but only when going in it directly.
The rest of services/ is made tier_app for browser (the same should be done for fennec ; the change is straightforward, but I can attach a patch if necessary)

I tested both ffx-on-xr and plain ffx builds with the two patches applied, and they both work, for some value of work: they both fail equally to login to the sync service. (it does get further than without this patch, i.e. it doesn't fail to load Cc['@labs.mozilla.com/Weave/Crypto;1']). It also fails equally to login with a plain ffx build without the patches applied, so this would be a separate issue.
Attachment #496795 - Flags: review?(mconnor)
Attachment #496794 - Flags: review?(mconnor) → review+
Comment on attachment 496795 [details] [diff] [review]
part 2 - Only build services-crypto component as part of platform

Okay, this'll do.
Attachment #496795 - Flags: review?(mconnor) → review+
Attached patch fennec partSplinter Review
cf. comment 4
Attachment #496880 - Flags: review?(mark.finkle)
I don't think this blocks b8, but should get into the builds ASAP.
blocking2.0: ? → beta9+
Attachment #496880 - Flags: review?(mark.finkle) → review+
I was getting the following error on try:
TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x80570018 (NS_ERROR_XPC_BAD_IID) [nsIJSCID.createInstance]"  nsresult: "0x80570018 (NS_ERROR_XPC_BAD_IID)"  location: "JS frame :: resource://services-sync/jpakeclient.js :: JPAKEClient :: line 139"  data: no]

It turns out the nsISyncJPAKE.xpt file correctly ended up in services-crypto.xpt, but later was overwritten when creating services-crypto.xpt for IWeaveCrypto.xpt...

The v2 separates both XPIDL_MODULEs, and adds the component xpt in the package manifest.

Pushed to try as 725eab14a160.
Attachment #496795 - Attachment is obsolete: true
Attachment #496924 - Flags: review?
This should be the last iteration. The jpake test needed Cc and Ci defined to work. It is defined in head_helper.js in services/crypto/tests/unit for the other tests but I didn't copy it over because it contains a reference to the application component and would thus fail running on a xulrunner build.

http://tbpl.mozilla.org/?tree=MozillaTry&rev=f157127e79e3
Attachment #496924 - Attachment is obsolete: true
Attachment #497049 - Flags: review?
Attachment #496924 - Flags: review?
Attachment #497049 - Flags: review? → review?(mconnor)
(In reply to comment #9)
> http://tbpl.mozilla.org/?tree=MozillaTry&rev=f157127e79e3

That got an all green. Mike, could you review this latest patch? Thanks.
Attachment #497049 - Flags: review?(mconnor) → review+
http://hg.mozilla.org/mozilla-central/rev/b31d9d9d8527
http://hg.mozilla.org/mozilla-central/rev/a7dea879b4b4

... and now I realize it should probably have landed on fx-sync first...

fennec part:
http://hg.mozilla.org/mobile-browser/rev/12565b572293
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(In reply to comment #11)
> ... and now I realize it should probably have landed on fx-sync first...

looks like the jpake code is not in fx-sync.
(In reply to comment #12)
> (In reply to comment #11)
> > ... and now I realize it should probably have landed on fx-sync first...
> 
> looks like the jpake code is not in fx-sync.

Indeed, it's trunk only. Thanks for taking care of this!
Fennec uses package manifests too. This makes the Fennec manifest match the Firefox manifest (landed in part 2)
Attachment #498817 - Flags: review?(mwu)
Attachment #498817 - Flags: review?(mwu) → review+
Whiteboard: [qa-]
As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)
blocking2.0: beta9+ → betaN+
No longer depends on: 601645
Depends on: 601645
Component: Firefox Sync: Crypto → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.