1421501 - Land Prio prototype in mozilla-central

Status: RESOLVED FIXED

(Toolkit :: General, enhancement, P3)

Description

[Robert Helmer [:rhelmer]]

We want to land a prototype PRIO client in Firefox.

The URL on this bug contains the paper and prototype Go code for the client and server.

Comment 1

[Robert Helmer [:rhelmer]]

Project description and some architectural notes:

https://docs.google.com/document/d/1NuV5bKZ8MHmGPUWraJbqMF_YQVvSigUNdvCywZtxRvs/edit?ts=5a1de142#

Comment 2

[Robert Helmer [:rhelmer]]

Attachment: Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

Review commit: https://reviewboard.mozilla.org/r/251948/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251948/

Comment 3

[Robert Helmer [:rhelmer]]

Attachment: Bug 1421501 - build system integration for libprio

Review commit: https://reviewboard.mozilla.org/r/251950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251950/

Comment 4

[Robert Helmer [:rhelmer]]

Attachment: Bug 1421501 - WebIDL and DOM for PrioEncoder

This is conspicuously missing tests... I am working on the upstream
Prio author on this now since it's a bit involved.

In the meantime I am hoping that you'll find enough wrong with my
patch that it's worth putting in front of you now.

Review commit: https://reviewboard.mozilla.org/r/251952/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/251952/

Comment 5

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

https://reviewboard.mozilla.org/r/251948/#review258384

Do you want me to review the code for correctness or just a general "this looks ok"?
Some first thougts:
I'm a little uneasy to ship such a big chunk of non-production-ready code. There is some dead code in there and a second copy of mpi (we that in NSS already, but it can't be used through NSS unfortunately). As the Readme says "I have no doubt that there are scary bugs, side-channel attacks, and memory leaks lurking herein".
So I'd be more comfortable with only vendoring the code that's needed/executed and maybe use the NSS copy of mpi (using a different include path shouldn't be a big issue).

Comment 6

[Robert Helmer [:rhelmer]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #5)
> Comment on attachment 8986636 [details]
> Bug 1421501 - add vendored libprio from
> https://github.com/henrycg/libprio/commit/
> 94e27d61967025848f7b24e3102cb310770689f0
> 
> https://reviewboard.mozilla.org/r/251948/#review258384
> 
> Do you want me to review the code for correctness or just a general "this
> looks ok"?
>
> Some first thougts:
> I'm a little uneasy to ship such a big chunk of non-production-ready code.
> There is some dead code in there and a second copy of mpi (we that in NSS
> already, but it can't be used through NSS unfortunately). As the Readme says
> "I have no doubt that there are scary bugs, side-channel attacks, and memory
> leaks lurking herein".

This will all be behind a pref initially so we can test and see if it really works, assuming everything goes OK we would like it to be reviewed for correctness.

> So I'd be more comfortable with only vendoring the code that's
> needed/executed and maybe use the NSS copy of mpi (using a different include
> path shouldn't be a big issue).

Thanks for taking a look! I'll forward this to the upstream author, I wonder if it wouldn't make more sense to have you review the upstream repo somehow (https://github.com/henrycg/libprio/)

Comment 8

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Julien, this patch requires two Curve25519 public keys, which are stored in string prefs ("prio.publicKeyA" and "prio.publicKeyB"). This is for Prio, which is a privacy-preserving system for the collection of aggregate statistics that we're testing for inclusion in Firefox.

I used python+libsodium to generate two keypairs for testing purposes and so I'd have something to put in the patch, however since we want to actually ship this how should we be handling the keys? Can you (or whoever should handle these) re-generate this in a way where the private key is kept safe and just send me the public keys?

The only place the private key should ever be needed is for decrypting the data coming into Telemetry (see bug 1465252)

For more background into this, please see the google doc linked from this bug with the project plan and also the Prio paper - the tl;dr is that Prio supports sending Telemetry data to different servers ("A" and "B") that can be run by different organizations that don't need to trust each other, but for our initially testing we will just be collecting everything from our one standard Telemetry endpoint and decrypting packets intended for the different servers during analysis.

If all goes well, we would eventually only manage one of these keypairs. The Prio paper is the best reference to explain the intent here: https://crypto.stanford.edu/prio/

Comment 9

[Henry Corrigan-Gibbs [:henrycg]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #5)
> Comment on attachment 8986636 [details]
> Bug 1421501 - add vendored libprio from
> https://github.com/henrycg/libprio/commit/
> 94e27d61967025848f7b24e3102cb310770689f0
> 
> https://reviewboard.mozilla.org/r/251948/#review258384
> 
> Do you want me to review the code for correctness or just a general "this
> looks ok"?
> Some first thougts:
> I'm a little uneasy to ship such a big chunk of non-production-ready code.
> There is some dead code in there and a second copy of mpi (we that in NSS
> already, but it can't be used through NSS unfortunately). As the Readme says
> "I have no doubt that there are scary bugs, side-channel attacks, and memory
> leaks lurking herein".
> So I'd be more comfortable with only vendoring the code that's
> needed/executed and maybe use the NSS copy of mpi (using a different include
> path shouldn't be a big issue).

This is the author of libprio here. 
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #5)
> Comment on attachment 8986636 [details]
> Bug 1421501 - add vendored libprio from
> https://github.com/henrycg/libprio/commit/
> 94e27d61967025848f7b24e3102cb310770689f0
> 
> https://reviewboard.mozilla.org/r/251948/#review258384
> 
> Do you want me to review the code for correctness or just a general "this
> looks ok"?
> Some first thougts:
> I'm a little uneasy to ship such a big chunk of non-production-ready code.
> There is some dead code in there and a second copy of mpi (we that in NSS
> already, but it can't be used through NSS unfortunately). As the Readme says
> "I have no doubt that there are scary bugs, side-channel attacks, and memory
> leaks lurking herein".
> So I'd be more comfortable with only vendoring the code that's
> needed/executed and maybe use the NSS copy of mpi (using a different include
> path shouldn't be a big issue).

This is the author of libprio here. Let me know if I can help out with any of this and/or answer any questions about the library.


(In reply to Robert Helmer [:rhelmer] from comment #6)
> (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #5)
> > Comment on attachment 8986636 [details]
> > Bug 1421501 - add vendored libprio from
> > https://github.com/henrycg/libprio/commit/
> > 94e27d61967025848f7b24e3102cb310770689f0
> > 
> > https://reviewboard.mozilla.org/r/251948/#review258384
> > 
> > Do you want me to review the code for correctness or just a general "this
> > looks ok"?
> >
> > Some first thougts:
> > I'm a little uneasy to ship such a big chunk of non-production-ready code.
> > There is some dead code in there and a second copy of mpi (we that in NSS
> > already, but it can't be used through NSS unfortunately). As the Readme says
> > "I have no doubt that there are scary bugs, side-channel attacks, and memory
> > leaks lurking herein".
> 
> This will all be behind a pref initially so we can test and see if it really
> works, assuming everything goes OK we would like it to be reviewed for
> correctness.
> 
> > So I'd be more comfortable with only vendoring the code that's
> > needed/executed and maybe use the NSS copy of mpi (using a different include
> > path shouldn't be a big issue).
> 
> Thanks for taking a look! I'll forward this to the upstream author, I wonder
> if it wouldn't make more sense to have you review the upstream repo somehow
> (https://github.com/henrycg/libprio/)

This is the author of libprio here. Let me know if there's any way I can help out with this review. I would be happy to have you review the libprio repo as well, if that's possible.

Comment 10

[Franziskus Kiefer [:franziskus]]

> we would like it to be reviewed for correctness.
> I wonder if it wouldn't make more sense to have you review the upstream repo somehow (https://github.com/henrycg/libprio/)

Given that this is all code from the repo I could also review it there. But it would be great if you/the upstream author could do some clean-up first such that only used code is there. That would make reviewing it easier.

Comment 11

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

https://reviewboard.mozilla.org/r/251948/#review259106

Cancelling this for now. Let me know how you want to proceed.

Comment 12

[Robert Helmer [:rhelmer]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

https://reviewboard.mozilla.org/r/251948/#review259106

Thanks, I've just discussed with Henry and he is going to prepare a PR against https://github.com/mozilla/libprio so we can separate the review of libprio itself vs. making sure it's vendored in m-c correctly.

Comment 13

[Gregory Szorc [:gps]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

https://reviewboard.mozilla.org/r/251950/#review259288

This looks good from a build system integration side.

I'm not the correct person to assess the quality of the C code for incorporation in Firefox. I'm guessing fkiefer is handling that as part of the vendoring commit...

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

Actually, let's wait for the vendoring bit to become more solidified. From the build system side, it looks like a rubber stamp review. I just don't want my r+ to be misconstrued as permission to vendor and to link into libxul.

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review258608

::: dom/chrome-webidl/PrioEncoder.webidl:7
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[Constructor(), ChromeOnly, Exposed=(Window,System,Worker)]

Do we need this on workers?

::: dom/chrome-webidl/PrioEncoder.webidl:10
(Diff revision 1)
> + */
> +
> +[Constructor(), ChromeOnly, Exposed=(Window,System,Worker)]
> +interface PrioEncoder {
> +  [Throws]
> +  DOMString getVersion();

Why is this a method rather than a readonly attribute?

::: dom/chrome-webidl/PrioEncoder.webidl:13
(Diff revision 1)
> +interface PrioEncoder {
> +  [Throws]
> +  DOMString getVersion();
> +
> +  [Throws]
> +  sequence<DOMString> getParameters();

Same question here. A `.parameters` attribute feels cleaner to me.

::: dom/prio/PrioEncoder.h:16
(Diff revision 1)
> +#include "mozilla/dom/TypedArray.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsWrapperCache.h"
> +#include "mozilla/dom/Promise.h"
> +#include "mprio.h"

Most of these forwards aren't necessary here, and should only be in the .cpp file :-).

::: dom/prio/PrioEncoder.h:25
(Diff revision 1)
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class PrioEncoder final : public nsISupports
> +                            , public nsWrapperCache

nit: indentation

::: dom/prio/PrioEncoder.cpp:28
(Diff revision 1)
> +PublicKey mPublicKeyB = nullptr;
> +
> +/* static */ already_AddRefed<PrioEncoder>
> +PrioEncoder::Constructor(const GlobalObject& aGlobal, ErrorResult& aRv)
> +{
> +  Prio_init();

I worry about this use in a constructor. Prio_init seems like it sets up some global internal state which is shared between multiple potential instances of PrioEncoder.

Perhaps it would make more sense to have PrioEncoder be a webidl namespace which initializes prio the first time any of its methods are accessed, and calls Prio_clear() when receiving the xpcom-shutdown observer notification.

::: dom/prio/PrioEncoder.cpp:31
(Diff revision 1)
> +PrioEncoder::Constructor(const GlobalObject& aGlobal, ErrorResult& aRv)
> +{
> +  Prio_init();
> +
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());
> +  MOZ_ASSERT(!global);

I think this is backwards. You want to assert that you have a global, not that you don't have one. Right?

::: dom/prio/PrioEncoder.cpp:51
(Diff revision 1)
> +
> +  // TODO The underlying Publickey_import function is a quick hack - should instead figure out how to turn the
> +  //      nsAutoString pubkey we're getting from prefs directly into an NSS PublicKey, instead.
> +
> +  SECStatus prio_rv = SECSuccess;
> +  prio_rv = PublicKey_import(&mPublicKeyA, reinterpret_cast<const uint8_t *>(ToNewCString(prioKeyA)), CURVE25519_KEY_LEN);

ToNewCString() allocates, so this will leak the string unless the PublicKey_import API takes ownership & frees for us (which should definitely warrant a comment)

Usually if you need a cstring from a wide string for a single call, you use NS_ConvertUTF16toUTF8(str).get().

::: dom/prio/PrioEncoder.cpp:57
(Diff revision 1)
> +  if (prio_rv != SECSuccess) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);
> +    return nullptr;
> +  }
> +
> +  prio_rv = PublicKey_import(&mPublicKeyB, reinterpret_cast<const uint8_t *>(ToNewCString(prioKeyB)), CURVE25519_KEY_LEN);

Same here

::: dom/prio/PrioEncoder.cpp:99
(Diff revision 1)
> +
> +void
> +PrioEncoder::GetParameters(nsTArray<nsString>& aParams,
> +                           ErrorResult& aRv)
> +{
> +  // TODO hardcoded, get keys from PrioParams dict instead

If we just want a list of properties from PrioParams, why are we doing this dynamic list? Do we need to dynamically expose this information to the consumer JS code?

To my understanding, this would only ever be exposed for chrome code which should already know the full set of keys it cares about by virtue of knowing the version of firefox it was built for.

::: dom/prio/PrioEncoder.cpp:106
(Diff revision 1)
> +  aParams.AppendElement(NS_LITERAL_STRING("hasCrashedInLast24Hours"));
> +  aParams.AppendElement(NS_LITERAL_STRING("hasDefaultHomepage"));
> +}
> +
> +already_AddRefed<Promise>
> +PrioEncoder::Encode(const nsAString& aBatchID, const PrioParams& aPrioParams, ErrorResult& aRv)

You can dodge the need to re-encode from wide to narrow strings if you just use a bytestring in the webidl layer. It won't support arbitrary unicode, but it appears as though you don't want that anyway (given that ToNewCString also doesn't support that).

::: dom/prio/PrioEncoder.cpp:117
(Diff revision 1)
> +
> +  unsigned char *forServerA = nullptr;
> +  unsigned char *forServerB = nullptr;
> +  unsigned int aLen, bLen;
> +
> +  const uint8_t *batchID = reinterpret_cast<const uint8_t *>(ToNewCString(aBatchID));

This allocates a value which doesn't appear to be freed again. 

If you can't do the bytestring thing, instead do:

NS_ConvertUTF16toUTF8 batchID(aBatchID);

// And then when you want to actually get information, use this like other nsCString objects:

PrioConfig_new(..., batchID.get(), batchID.Length());

::: dom/prio/PrioEncoder.cpp:118
(Diff revision 1)
> +  unsigned char *forServerA = nullptr;
> +  unsigned char *forServerB = nullptr;
> +  unsigned int aLen, bLen;
> +
> +  const uint8_t *batchID = reinterpret_cast<const uint8_t *>(ToNewCString(aBatchID));
> +  const unsigned int batchIDLen = aBatchID.Length();

If the string contains non-ascii characters, using the length of the wide string will probably be inaccurate for the narrow string.

ToNewCString does a fairly lossy conversion so it probably won't be a problem for you, but it's still not great style.

::: dom/prio/PrioEncoder.cpp:120
(Diff revision 1)
> +  unsigned int aLen, bLen;
> +
> +  const uint8_t *batchID = reinterpret_cast<const uint8_t *>(ToNewCString(aBatchID));
> +  const unsigned int batchIDLen = aBatchID.Length();
> +
> +  bool dataItems[sizeof(aPrioParams)];

sizeof is really gross & unstable here :-/.

Can we instead handle it like this:

AutoTArray<bool> dataItems;

// Add each of our parameters.
dataItems.AppendElement(aPrioParams.mHasCrashedInLast24Hours);
dataItems.AppendElement(aPrioParams.mHasDefaultHomepage);
dataItems.AppendElement(aPrioParams.mHasUsedPrivateBrowsing);

// Create our prioconfig
PrioConfig cfg = PrioConfig_new(dataItems.Length(), mPublicKeyA, mPublicKeyB, batchID.get(), batchID.Length());
// And call encode
PrioClient_encode(cfg, dataItems.Elements(), &forServerA, &aLen, &forServerB, &bLen);

::: dom/prio/PrioEncoder.cpp:130
(Diff revision 1)
> +
> +    dataItems[0] = aPrioParams.mHasCrashedInLast24Hours;
> +    dataItems[1] = aPrioParams.mHasDefaultHomepage;
> +    dataItems[2] = aPrioParams.mHasUsedPrivateBrowsing;
> +
> +    rv = PrioClient_encode(cfg, dataItems, &forServerA, &aLen, &forServerB, &bLen);

Took a peek at the docs for PrioClient. It looks like forServerA and forServerB should be freed after this method is called, but I don't see any calls to `free` in this method.

::: dom/prio/PrioEncoder.cpp:147
(Diff revision 1)
> +  nsTArray<uint8_t> resultB;
> +  for (size_t i=0; i < bLen; i++) {
> +    resultB.AppendElement(forServerB[i]);
> +  }
> +
> +  nsTArray<nsTArray<uint8_t>> ret;

This double-nested array of integers setup seems a touch inconvenient, and also memory-inefficient. Do you have a need to have it in this specific format?

Currently each of the elements in this array will allocate 64-bits of data (as that is the size of a JS value) which is unfortunate. What sort of stuff do you intend to do with this data?

::: dom/prio/PrioEncoder.cpp:152
(Diff revision 1)
> +  nsTArray<nsTArray<uint8_t>> ret;
> +
> +  ret.AppendElement(resultA);
> +  ret.AppendElement(resultB);
> +
> +  promise->MaybeResolve(ret);

It looks like you always resolve & reject the promise synchronously? Is the end goal to do this work off-main-thread or something like that?

::: dom/prio/moz.build:10
(Diff revision 1)
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +with Files("**"):
> +    BUG_COMPONENT = ("Core", "DOM")
> +
> +# TEST_DIRS += ['tests']

Why is this line here & commented out?

Comment 16

[Julien Vehent]

For future ref, we had a chat today with rhelmer, mreid, ameihm and I about the operational security of this experiments. Notes are at https://docs.google.com/document/d/1C7cb1YXbYFkElEDOMFnztni_qrmMerhwpvy-hu2c47Y/edit#

In short: data ops should generate the two keys, store them encrypted in conf, and send the public keys to rhelmer. A new job (not yet defined) will decrypt PRIO payloads inside the telemetry infrastructure and will need access to the private keys.

Comment 17

[Tom Ritter [:tjr]]

Also for future ref: We are creating a new Security Review process and Rob was kind enough to be a guinea pig. The review took place in https://docs.google.com/document/d/1ru10QrZo8L7iXKWbx3OehbkmdkljTqCVLsjQlItTrW4/edit#

Comment 18

[Robert Helmer [:rhelmer]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #11)
> Comment on attachment 8986636 [details]
> Bug 1421501 - add vendored libprio from
> https://github.com/henrycg/libprio/commit/
> 94e27d61967025848f7b24e3102cb310770689f0
> 
> https://reviewboard.mozilla.org/r/251948/#review259106
> 
> Cancelling this for now. Let me know how you want to proceed.

BTW this review happened in https://github.com/mozilla/libprio/pull/1 and was merged into the mozilla/libprio repo. I'll update the patch on this bug.

Comment 19

[Robert Helmer [:rhelmer]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251948/diff/1-2/

Comment 20

[Robert Helmer [:rhelmer]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251950/diff/1-2/

Comment 21

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/1-2/

Comment 22

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review258608

> If we just want a list of properties from PrioParams, why are we doing this dynamic list? Do we need to dynamically expose this information to the consumer JS code?
> 
> To my understanding, this would only ever be exposed for chrome code which should already know the full set of keys it cares about by virtue of knowing the version of firefox it was built for.

I think it is just for the convenience of the JS consumer, but I see your point. I think we could just avoid doing the `parameters` method and have folks refer to the webidl, to figure out which telemetry values are supported.

> sizeof is really gross & unstable here :-/.
> 
> Can we instead handle it like this:
> 
> AutoTArray<bool> dataItems;
> 
> // Add each of our parameters.
> dataItems.AppendElement(aPrioParams.mHasCrashedInLast24Hours);
> dataItems.AppendElement(aPrioParams.mHasDefaultHomepage);
> dataItems.AppendElement(aPrioParams.mHasUsedPrivateBrowsing);
> 
> // Create our prioconfig
> PrioConfig cfg = PrioConfig_new(dataItems.Length(), mPublicKeyA, mPublicKeyB, batchID.get(), batchID.Length());
> // And call encode
> PrioClient_encode(cfg, dataItems.Elements(), &forServerA, &aLen, &forServerB, &bLen);

Huh,S s I agree this is much nicer, but it's crashing on `PrioConfig_new(dataItems.Length(), [...])` and I haven't had time to debug it :/ This happens with both `AutoTArray` and `nsTArray`.

Just for the moment I hardcoded the number of data items here since we know what they are and this is a prototype, but I will spend some time figuring out what's wrong here.

> This double-nested array of integers setup seems a touch inconvenient, and also memory-inefficient. Do you have a need to have it in this specific format?
> 
> Currently each of the elements in this array will allocate 64-bits of data (as that is the size of a JS value) which is unfortunate. What sort of stuff do you intend to do with this data?

I think really we want to do an array-of-arraybuffers here, but I am having trouble getting this working... naively trying to return something like `nsTArray<ArrayBuffer>` doesn't seem to be supported, still trying to figure this out.

The only thing we're likely to do with the data is encode it in some way that it can be placed into the Telemetry packet (which is current JSON I believe).

Do you havae any thoughts on what would be better here?

> It looks like you always resolve & reject the promise synchronously? Is the end goal to do this work off-main-thread or something like that?

Eventually yes, I think for the prototype it's not critical, but we ultimately don't want to block the main thread to do the encoding, if we end up using it for very many Telemetry values. I went with a promise here in the interest of finalizing the API, but happy to change this to be a plain synchronous function if you'd prefer.

Comment 23

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review258608

> This allocates a value which doesn't appear to be freed again. 
> 
> If you can't do the bytestring thing, instead do:
> 
> NS_ConvertUTF16toUTF8 batchID(aBatchID);
> 
> // And then when you want to actually get information, use this like other nsCString objects:
> 
> PrioConfig_new(..., batchID.get(), batchID.Length());

This compiles but crashes for me as well :/ Sorry I am not very experienced at wrangling Mozilla's string types into something that can be used with C, I could use some help here.

Comment 24

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/2-3/

Comment 25

[Robert Helmer [:rhelmer]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251948/diff/2-3/

Comment 26

[Robert Helmer [:rhelmer]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251950/diff/2-3/

Comment 27

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/3-4/

Comment 28

[Robert Helmer [:rhelmer]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

So, I've found a snag with this just now testing on tryserver - we introduced a dependency on msgpack in the upstream libprio since the last time I did a try build.

Should msgpack be vendored in m-c, or can we make it a build dependency?

Comment 29

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #28)
> Comment on attachment 8986637 [details]
> Bug 1421501 - build system integration for libprio
> 
> So, I've found a snag with this just now testing on tryserver - we
> introduced a dependency on msgpack in the upstream libprio since the last
> time I did a try build.
> 
> Should msgpack be vendored in m-c, or can we make it a build dependency?

Comment 30

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

https://reviewboard.mozilla.org/r/251948/#review268186

Didn't you want to use the in-tree copy of mpi?

Comment 31

[Robert Helmer [:rhelmer]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

https://reviewboard.mozilla.org/r/251948/#review268186

Oh good catch thanks!

So I think we should import the libprio repo in full but specify via our moz.build (the second commit in the series here), does that make sense?

I want to keep the import part as simple as possible.

Comment 32

[Gregory Szorc [:gps]]

msgpack should be vendored in m-c. I'm assuming we'll also want to add a --with-system-msgpack configure option, as downstream packagers will want the option to unbundle our msgpack and use the system library. It's what they do.

I'm sure glandium will weigh in if I'm incorrect about this expectation.

Comment 33

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review268332

::: dom/chrome-webidl/PrioEncoder.webidl:9
(Diff revision 4)
> +  [Throws]
> +  readonly attribute DOMString version;

It looks from the implementation that this method never throws. Please remove the 'Throws' annotation :-)

::: dom/moz.build:66
(Diff revision 4)
>      'media',
>      'midi',
>      'notification',
>      'offline',
>      'power',
> +    'prio',

Do we really want a whole directory for this one file?

It feels a touch to me like this should be part of the telemetry component & just exposed through XPIDL to chrome pages.

::: dom/prio/PrioEncoder.cpp:17
(Diff revision 4)
> +PublicKey mPublicKeyA = nullptr;
> +PublicKey mPublicKeyB = nullptr;
> +
> +PrioConfig mPrioConfig = nullptr;

Please make these private static properties on the Prio object itself. They should also be named with an 's' rather than a 'm', as they are not members.

::: dom/prio/PrioEncoder.cpp:22
(Diff revision 4)
> +class ClearPrioOnShutdown final : public nsIObserver {
> +  ~ClearPrioOnShutdown() {}
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIOBSERVER
> +};
> +
> +NS_IMPL_ISUPPORTS(ClearPrioOnShutdown, nsIObserver)
> +
> +NS_IMETHODIMP
> +ClearPrioOnShutdown::Observe(nsISupports* aSubject, const char* aTopic, const char16_t* data)
> +{
> +  MOZ_ASSERT(!strcmp(aTopic, "xpcom-shutdown"));
> +
> +  if (mPublicKeyA) PublicKey_clear (mPublicKeyA);
> +  mPublicKeyA = nullptr;
> +
> +  if (mPublicKeyB) PublicKey_clear (mPublicKeyB);
> +  mPublicKeyB = nullptr;
> +
> +  mPrioConfig = nullptr;
> +
> +  return NS_OK;
> +}

We should move the shared state into a StaticAutoPtr struct which is initialized when we need it. We can then skip this code and just use mozilla::ClearOnShutdown (https://searchfox.org/mozilla-central/rev/e52cd92858800a69b74cb97d26d9bdb960d611ca/xpcom/base/ClearOnShutdown.h#96-116) to clear the pointer & free the backing memory when the browser is shutting down.

::: dom/prio/PrioEncoder.cpp:59
(Diff revision 4)
> +/* static */
> +already_AddRefed<Promise>
> +PrioEncoder::Encode(GlobalObject& aGlobal, const nsAString& aBatchID, const PrioParams& aPrioParams, ErrorResult& aRv)
> +{
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(aGlobal.GetAsSupports());
> +  MOZ_ASSERT(global);

Please handle global being nullptr, and just raise an error :-).

::: dom/prio/PrioEncoder.cpp:63
(Diff revision 4)
> +  if (!mPrioConfig) {
> +    Prio_init();
> +
> +    nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
> +    if (obs) {
> +      obs->AddObserver(new ClearPrioOnShutdown(), "xpcom-shutdown", false);
> +    }
> +
> +    nsAutoString prioKeyA;
> +    nsresult rv = Preferences::GetString("prio.publicKeyA", prioKeyA);
> +    if (NS_FAILED(rv)) {
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +
> +    nsAutoString prioKeyB;
> +    rv = Preferences::GetString("prio.publicKeyB", prioKeyB);
> +    if (NS_FAILED(rv)) {
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +
> +    NS_ConvertUTF16toUTF8 prefStringKeyA(prioKeyA);
> +    NS_ConvertUTF16toUTF8 prefStringKeyB(prioKeyB);
> +
> +    // Check that both public keys are of the right length
> +    // and contain only hex digits 0-9a-fA-f
> +    if (!PrioEncoder::IsValidHexPublicKey(prefStringKeyA.get(), prefStringKeyA.Length())
> +        || !PrioEncoder::IsValidHexPublicKey(prefStringKeyB.get(), prefStringKeyB.Length()))  {
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +
> +    if (mPublicKeyA) PublicKey_clear (mPublicKeyA);
> +    prio_rv = PublicKey_import_hex(&mPublicKeyA, reinterpret_cast<const uint8_t *>(prefStringKeyA.get()), CURVE25519_KEY_LEN_HEX);
> +    if (prio_rv != SECSuccess) {
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +
> +    if (mPublicKeyB) PublicKey_clear (mPublicKeyB);
> +    prio_rv = PublicKey_import_hex(&mPublicKeyB, reinterpret_cast<const uint8_t *>(prefStringKeyB.get()), CURVE25519_KEY_LEN_HEX);
> +    if (prio_rv != SECSuccess) {
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +  }

This code should check the StaticAutoPtr, and create an object with these fields if needed, calling ClearOnShutdown() to set up the cleanup :-).

::: dom/prio/PrioEncoder.cpp:115
(Diff revision 4)
> +
> +  RefPtr<Promise> promise = Promise::Create(global, aRv);
> +
> +  unsigned char *forServerA = nullptr;
> +  unsigned char *forServerB = nullptr;
> +  unsigned int aLen, bLen;

Please initialize these outparams, and only define them when we're actually going to use them (a.k.a. just before PrioClient_encode)

::: dom/prio/PrioEncoder.cpp:117
(Diff revision 4)
> +  const uint8_t *batchID = reinterpret_cast<const uint8_t *>(ToNewCString(aBatchID));
> +  const unsigned int batchIDLen = aBatchID.Length();

This call currently leaks memory and doesn't handle unicode values nicely, which is unfortunate.

Please do one of the following:
 1. Change the function to take a ByteString instead of a DOMString, to explicitly note that you only support byte values. Then:
   a) Pass `aBatchID.BeginReading()` for the `const char*` pointer argument. You may have to explicitly cast to `const uint8_t*` if the API takes that (oh well).
   b) Pass `aBatchID.Length()` directly for the length argument.
 2. If we intend to support unicode in batch IDs
   a) re-encode it to UTF8 first, by calling `NS_ConvertUTF16toUTF8 batchID(aBatchID);`
   b) `batchID` is now the right kind of string and should be able to be passed directly as in 1.

::: dom/prio/PrioEncoder.cpp:120
(Diff revision 4)
> +  const unsigned int nDataItems = 3;
> +  bool dataItems[nDataItems];
> +
> +  dataItems[0] = aPrioParams.mStartupCrashDetected;
> +  dataItems[1] = aPrioParams.mSafeModeUsage;
> +  dataItems[2] = aPrioParams.mBrowserIsUserDefault;

You can declare this as:

bool dataItems[] = {
  aPrioParams.mStartupCrashDetected,
  aPrioParams.mSafeModeUsage,
  aPrioParams.mBrowserIsUserDefault
};

and get the number automatically with 'mozilla::ArrayLength' (https://searchfox.org/mozilla-central/rev/e52cd92858800a69b74cb97d26d9bdb960d611ca/mfbt/ArrayUtils.h#49-60)

::: dom/prio/PrioEncoder.cpp:135
(Diff revision 4)
> +  if (!mPrioConfig) {
> +    promise->MaybeReject(NS_ERROR_FAILURE);
> +    return promise.forget();
> +  }
> +
> +  prio_rv = PrioClient_encode(mPrioConfig, dataItems, &forServerA, &aLen, &forServerB, &bLen);

I think the type you're resolving the promise with is slightly unfortunate here. It would be cleaner to instead return a dictionary defined in WebIDL, such as:

    dictionary PrioEncodedData {
      ByteString a;
      ByteString b;
    }
    
We can then make it much easier to work with our returned encoded data by directly moving ownership into the bytestring objects, for example:

    uint8_t* forA = nullptr;
    uint32_t lenA = 0;
    uint8_t* forB = nullptr;
    uint32_t lenB = 0;
    prio_rv = PrioClient_encode(mPrioConfig, dataItems, &forA, &lenA, &forB, &lenB);
    
    // Package the data into the dictionary
    // The 'Adopt' calls take ownership, so we don't have to explicitly free the pointers anymore!
    PrioEncodedData data;
    data.mA.Adopt((char*)forA, lenA);
    data.mB.Adopt((char*)forB, lenB);
    
    if (prio_rv != SECSuccess) {
      return Promise::Reject(global, cx, JS::UndefinedHandleValue(), aRv);
    }

You can then even avoid building those nsTArrays. In JS code it is probably more convenient to deal with them as ByteString objects as-is (also they're much faster & more memory-efficient), so probably return them as ByteString objects in the dictionary :-).

::: dom/prio/PrioEncoder.cpp:155
(Diff revision 4)
> +  nsTArray<uint8_t> resultB;
> +  for (size_t i=0; i < bLen; i++) {
> +    resultB.AppendElement(forServerB[i]);
> +  }
> +
> +  if (mPrioConfig) PrioConfig_clear(mPrioConfig);

If you're going to clear this at the end of the function anyway, why are you making it global in the first place?

::: dom/prio/PrioEncoder.cpp:172
(Diff revision 4)
> +  if (strlen != CURVE25519_KEY_LEN_HEX)
> +    return false;

nit: please always put curlies around blocks like this.

::: dom/prio/PrioEncoder.cpp:175
(Diff revision 4)
> +PrioEncoder::IsValidHexPublicKey(const char *str, size_t strlen)
> +{
> +  if (strlen != CURVE25519_KEY_LEN_HEX)
> +    return false;
> +
> +  for (size_t i=0; i<strlen; i++) {

nit: spaces around operators

::: dom/prio/PrioEncoder.cpp:176
(Diff revision 4)
> +    if (!std::isxdigit(str[i]))
> +      return false;

curlies.

Comment 34

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #32)
> msgpack should be vendored in m-c. I'm assuming we'll also want to add a
> --with-system-msgpack configure option, as downstream packagers will want
> the option to unbundle our msgpack and use the system library. It's what
> they do.
> 
> I'm sure glandium will weigh in if I'm incorrect about this expectation.

Meh. We have so many things that are bundled and without a --with-system-... thing. If downstreams really want that, they can submit patches. And I'm writing this as a downstream myself.

As someone who also spent a large amount of time to make Android LTO a possibility and having had to fix mpi for that, please ensure the mpi copy in libprio is up-to-date with nss trunk.

Comment 35

[Robert Helmer [:rhelmer]]

(In reply to Mike Hommey [:glandium] from comment #34)
> (In reply to Gregory Szorc [:gps] from comment #32)
> > msgpack should be vendored in m-c. I'm assuming we'll also want to add a
> > --with-system-msgpack configure option, as downstream packagers will want
> > the option to unbundle our msgpack and use the system library. It's what
> > they do.
> > 
> > I'm sure glandium will weigh in if I'm incorrect about this expectation.
> 
> Meh. We have so many things that are bundled and without a --with-system-...
> thing. If downstreams really want that, they can submit patches. And I'm
> writing this as a downstream myself.
> 
> As someone who also spent a large amount of time to make Android LTO a
> possibility and having had to fix mpi for that, please ensure the mpi copy
> in libprio is up-to-date with nss trunk.

Hm so there are two things here I thing:

1) msgpack, which we don't have in-tree at all AFAIK (and isn't bundled in libprio but could be)

I think this is the one where the "--with-system" issue would come in, however per the libprio README we need a very recent version of msgpack so I expect that most distro versions may not be new enough yet, anyway.

So I think we should just vendor this. It's Apache licensed and really tiny, I already have a working patch for this, will add a new commit to the bug soon. I think given the above "--with-system" probably isn't likely to work currently, anyway.

2) mpi is copied from NSS into libprio - I think this is just a matter of using the in-tree header and linking against freebl3 (although I haven't got this to work quite yet), I'd rather do this than use a copy inside libprio, just from a maintenance perspective.

Does this all sound reasonable?

Comment 36

[Mike Hommey [:glandium]]

1) That means you *have* to vendor anyways, because we can't have our (mozilla.org) Firefox builds depend on a library that is barely available on user systems.

2) I don't think freebl is exposing the mpi symbols.

Comment 37

[Robert Helmer [:rhelmer]]

(In reply to Mike Hommey [:glandium] from comment #36)
> 1) That means you *have* to vendor anyways, because we can't have our
> (mozilla.org) Firefox builds depend on a library that is barely available on
> user systems.

Cool, agreed.

> 2) I don't think freebl is exposing the mpi symbols.

Oh, hm. Well that makes it problematic to use the in-tree NSS then, should we stick with the copy?

Comment 38

[Franziskus Kiefer [:franziskus]]

> So I think we should import the libprio repo in full but specify via our moz.build (the second commit in the series here), does that make sense?
> I want to keep the import part as simple as possible.

Having dead third-party library code isn't great. I'd be fine with importing everything and then deleting the mpi folder again. That shouldn't be too much extra work :)

> > 2) I don't think freebl is exposing the mpi symbols.
> Oh, hm. Well that makes it problematic to use the in-tree NSS then, should we stick with the copy?

Right, mpi isn't exported from freebl/nss. That's why I suggested using the source code from NSS, not the NSS build. NSS has an option to expose all symbols (including mpi). But that's really only supposed for testing and nothing we should use in Firefox.
I think the best option is to allow setting the mpi source path in the libprio build. This way libprio can be built stand-alone but use the NSS copy of mpi as part of the Firefox build. Let me know if you need help with that.

Comment 39

[Robert Helmer [:rhelmer]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #38)
> > So I think we should import the libprio repo in full but specify via our moz.build (the second commit in the series here), does that make sense?
> > I want to keep the import part as simple as possible.
> 
> Having dead third-party library code isn't great. I'd be fine with importing
> everything and then deleting the mpi folder again. That shouldn't be too
> much extra work :)
> 
> > > 2) I don't think freebl is exposing the mpi symbols.
> > Oh, hm. Well that makes it problematic to use the in-tree NSS then, should we stick with the copy?
> 
> Right, mpi isn't exported from freebl/nss. That's why I suggested using the
> source code from NSS, not the NSS build. NSS has an option to expose all
> symbols (including mpi). But that's really only supposed for testing and
> nothing we should use in Firefox.
> I think the best option is to allow setting the mpi source path in the
> libprio build. This way libprio can be built stand-alone but use the NSS
> copy of mpi as part of the Firefox build. Let me know if you need help with
> that.

Ah, got it thanks.

Comment 40

[Robert Helmer [:rhelmer]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #38)
> I think the best option is to allow setting the mpi source path in the
> libprio build. This way libprio can be built stand-alone but use the NSS
> copy of mpi as part of the Firefox build. Let me know if you need help with
> that.

OK I have this compiling using the NSS copy of mpi now, by just changing the moz.build file, although I'll need to modify the upstream libprio and re-vendor that so we include the correct header file (libprio uses its own build system (scons) and I think really the source files should #include <mpi/mpi.h> instead of "mpi/mpi.h" and then either scons or mozbuild will put the correct location to mpi.h on the include path)

One thing that was throwing me here is that Henry had modified primes.c to #include mpi.h, but we don't need to build that directly because primes.c is included in mpprime.c (which includes mpi.h). I'll modify the builds script for both libprio and m-c to remove this confusion.

Anyway thanks for catching that and the explanation! I had glossed over that in your github review. I have it working locally now and will push for re-review once I've re-vendored for the above.

Comment 41

[Henry Corrigan-Gibbs [:henrycg]]

This all sounds good to me. Let me know if you need help with anything on the libprio side.

Comment 42

[Robert Helmer [:rhelmer]]

(In reply to Henry Corrigan-Gibbs from comment #41)
> This all sounds good to me. Let me know if you need help with anything on
> the libprio side.

Thanks for all your help! I'll have a libprio PR for you to review soon to address comment 40.

I'm also working through some minor issues I'm hitting compiling on Linux and Windows in our CI environment (macOS is fine), they look like the usual sort of cross-platform pains so I'll do a separate PR for these.

Comment 43

[Robert Helmer [:rhelmer]]

Attachment: Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review commit: https://reviewboard.mozilla.org/r/261616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261616/

Comment 44

[Robert Helmer [:rhelmer]]

Attachment: Bug 1421501 - build integration for vendored msgpack

Review commit: https://reviewboard.mozilla.org/r/261618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261618/

Comment 45

[Robert Helmer [:rhelmer]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251948/diff/3-4/

Comment 46

[Robert Helmer [:rhelmer]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251950/diff/3-4/

Comment 47

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/4-5/

Comment 48

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review268332

> Do we really want a whole directory for this one file?
> 
> It feels a touch to me like this should be part of the telemetry component & just exposed through XPIDL to chrome pages.

Hm, I did consider this when starting the project... I recall the issue coming up in https://mail.mozilla.org/pipermail/firefox-dev/2018-March/006161.html and I wasn't really sure if there was a great conclusion if we should be using XPIDL or WebIDL going forward.

Since this is a prototype and we'd like to start getting data so we can work on the backend I'd like to continue with what we have as it's feeling close now, but if you have strong feelings I am happy to move this over.

I think the point about having a whole dir is a good one though, I notice things like `ChromeUtils` are in dom/base so if you agree with me on the above then I could move it there.

> This code should check the StaticAutoPtr, and create an object with these fields if needed, calling ClearOnShutdown() to set up the cleanup :-).

Hope I did that right, took me a while to figure out how that works from existing code...

> This call currently leaks memory and doesn't handle unicode values nicely, which is unfortunate.
> 
> Please do one of the following:
>  1. Change the function to take a ByteString instead of a DOMString, to explicitly note that you only support byte values. Then:
>    a) Pass `aBatchID.BeginReading()` for the `const char*` pointer argument. You may have to explicitly cast to `const uint8_t*` if the API takes that (oh well).
>    b) Pass `aBatchID.Length()` directly for the length argument.
>  2. If we intend to support unicode in batch IDs
>    a) re-encode it to UTF8 first, by calling `NS_ConvertUTF16toUTF8 batchID(aBatchID);`
>    b) `batchID` is now the right kind of string and should be able to be passed directly as in 1.

Unicode isn't important here.

So approach 1 lgtm works with `DOMString`, but if I try it with `ByteString` compile fails with:
  error: no viable conversion from 'const nsTString<char>' to 'const nsAString' (aka 'const nsTSubstring<char16_t>')
  
I've left a FIXME in the WebIDL if you want to try it :)

> I think the type you're resolving the promise with is slightly unfortunate here. It would be cleaner to instead return a dictionary defined in WebIDL, such as:
> 
>     dictionary PrioEncodedData {
>       ByteString a;
>       ByteString b;
>     }
>     
> We can then make it much easier to work with our returned encoded data by directly moving ownership into the bytestring objects, for example:
> 
>     uint8_t* forA = nullptr;
>     uint32_t lenA = 0;
>     uint8_t* forB = nullptr;
>     uint32_t lenB = 0;
>     prio_rv = PrioClient_encode(mPrioConfig, dataItems, &forA, &lenA, &forB, &lenB);
>     
>     // Package the data into the dictionary
>     // The 'Adopt' calls take ownership, so we don't have to explicitly free the pointers anymore!
>     PrioEncodedData data;
>     data.mA.Adopt((char*)forA, lenA);
>     data.mB.Adopt((char*)forB, lenB);
>     
>     if (prio_rv != SECSuccess) {
>       return Promise::Reject(global, cx, JS::UndefinedHandleValue(), aRv);
>     }
> 
> You can then even avoid building those nsTArrays. In JS code it is probably more convenient to deal with them as ByteString objects as-is (also they're much faster & more memory-efficient), so probably return them as ByteString objects in the dictionary :-).

This looks much better than what I am doing! I am totally fine returning a pair of `ByteString`s instead of a pair of `Array`s.

However if I try to use `.Adopt` here then compilation fails with:
  error: no member named 'Adopt' in 'mozilla::dom::Optional<nsTString<char> >'
  
I don't feel like I have a good handle on the different string types and how ByteString fits into this :/ I've left the code commented out so hopefully you'll see what I mean :)

Comment 49

[Robert Helmer [:rhelmer]]

Henry was concerned about the way he's doing NSS init/shutdown, I know you already reviewed this code but would you mind looking over this PR?

https://github.com/mozilla/libprio/pull/2/files

It looks like we don't export the NSS_InitContext / NSS_ShutdownContext symbols, so I just want to double-check that the way this code is doing NSS_NoDB_Init / NSS_Shutdown is OK or if we should skip these entirely when using this in Firefox?

I took a look at the original github PR that you reviewed and didn't see this covered but possible I missed it. Thanks!

Comment 50

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #49)
> Henry was concerned about the way he's doing NSS init/shutdown, I know you
> already reviewed this code but would you mind looking over this PR?
> 
> https://github.com/mozilla/libprio/pull/2/files
> 
> It looks like we don't export the NSS_InitContext / NSS_ShutdownContext
> symbols, so I just want to double-check that the way this code is doing
> NSS_NoDB_Init / NSS_Shutdown is OK or if we should skip these entirely when
> using this in Firefox?
> 
> I took a look at the original github PR that you reviewed and didn't see
> this covered but possible I missed it. Thanks!

Oops nevermind - Henry spotted this, we need to move the `extern "C"` in https://github.com/mozilla/libprio/blob/5779b9cde2e21786d4cf55823e021b8c172990f0/include/mprio.h#L20 up a little bit.

Comment 51

[Mike Hommey [:glandium]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

https://reviewboard.mozilla.org/r/261616/#review269062

I don't know where you got this, but it doesn't match version 3.0.1 on https://github.com/msgpack/msgpack-c
Note that anything that is not autovendored (like rust with cargo vendor) should come with an update script and/or a readme saying where it comes from, and at what exact revision if it comes from a VCS (avoiding tags and prefering sha1s).

Comment 52

[Mike Hommey [:glandium]]

We should also probably prefer a rust implementation of msgpack. If it doesn't have a msgpack-c compatibility layer, that can probably be easily written.

Comment 53

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

https://reviewboard.mozilla.org/r/261616/#review269062

This was from a release: https://github.com/msgpack/msgpack-c/releases/tag/cpp-3.0.1

Comment 54

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

https://reviewboard.mozilla.org/r/261616/#review269062

I'll go ahead and do the update script, I see examples of this in other parts of the tree (doing a bespoke update script for every third-party repo is quite a pain, especially if the "happy path" is "lives on github and doesn't need any local patches" etc. but I am sure you know that :) )

Comment 55

[Robert Helmer [:rhelmer]]

(In reply to Mike Hommey [:glandium] from comment #52)
> We should also probably prefer a rust implementation of msgpack. If it
> doesn't have a msgpack-c compatibility layer, that can probably be easily
> written.

This is a good idea and I will take a look, I think we should probably defer this to a follow-up bug both because this is a prototype (we're only going to enable it for a small subset of users via feature-flag) and the important bit that is blocking folks is to start getting Prio-encoded Telemetry data in from real Nightly users.

That said, I will give it a try and if it just works then let's do it now.

Comment 56

[Robert Helmer [:rhelmer]]

BTW about to amend the patches on this bug to pull in some fixes to get libprio building on all platforms, thanks to help from Franziskus and Henry in https://github.com/mozilla/libprio/issues/3 and https://github.com/mozilla/libprio/issues/4 (re-vendoring libprio and some small moz.build tweaks).

Debugging this took quite a few comments which I wanted to spare everyone on this bug, but if interested and for posterity see the above.

Comment 57

[Robert Helmer [:rhelmer]]

Attachment: Bug 1421501 - export NSS [Init,Shutdown]Context symbols

Review commit: https://reviewboard.mozilla.org/r/261664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261664/

Comment 58

[Robert Helmer [:rhelmer]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251948/diff/4-5/

Comment 59

[Robert Helmer [:rhelmer]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251950/diff/4-5/

Comment 60

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261616/diff/1-2/

Comment 61

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261618/diff/1-2/

Comment 62

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/5-6/

Comment 63

[Robert Helmer [:rhelmer]]

(In reply to Mike Hommey [:glandium] from comment #51)
> Comment on attachment 8998386 [details]
> Bug 1421501 - vendor msgpack 3.0.1
> 
> https://reviewboard.mozilla.org/r/261616/#review269062
> 
> I don't know where you got this, but it doesn't match version 3.0.1 on
> https://github.com/msgpack/msgpack-c
> Note that anything that is not autovendored (like rust with cargo vendor)
> should come with an update script and/or a readme saying where it comes
> from, and at what exact revision if it comes from a VCS (avoiding tags and
> prefering sha1s).

Hm, actually could you please point me to an update script I can use as an example of the sort of script you'd like to see? I see quite a few different approaches in the tree today.

I don't really have strong feelings on the contents of such a script, but I can't imagine it needing to be much more complicated than a clone and copying w/o the git metadata into the third_party/msgpack/ dir (maybe using `git archive`), right?

Comment 64

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #63)
> (In reply to Mike Hommey [:glandium] from comment #51)
> > Comment on attachment 8998386 [details]
> > Bug 1421501 - vendor msgpack 3.0.1
> > 
> > https://reviewboard.mozilla.org/r/261616/#review269062
> > 
> > I don't know where you got this, but it doesn't match version 3.0.1 on
> > https://github.com/msgpack/msgpack-c
> > Note that anything that is not autovendored (like rust with cargo vendor)
> > should come with an update script and/or a readme saying where it comes
> > from, and at what exact revision if it comes from a VCS (avoiding tags and
> > prefering sha1s).
> 
> Hm, actually could you please point me to an update script I can use as an
> example of the sort of script you'd like to see? I see quite a few different
> approaches in the tree today.
> 
> I don't really have strong feelings on the contents of such a script, but I
> can't imagine it needing to be much more complicated than a clone and
> copying w/o the git metadata into the third_party/msgpack/ dir (maybe using
> `git archive`), right?

Comment 65

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #64)
> (In reply to Robert Helmer [:rhelmer] from comment #63)
> > (In reply to Mike Hommey [:glandium] from comment #51)
> > > Comment on attachment 8998386 [details]
> > > Bug 1421501 - vendor msgpack 3.0.1
> > > 
> > > https://reviewboard.mozilla.org/r/261616/#review269062
> > > 
> > > I don't know where you got this, but it doesn't match version 3.0.1 on
> > > https://github.com/msgpack/msgpack-c
> > > Note that anything that is not autovendored (like rust with cargo vendor)
> > > should come with an update script and/or a readme saying where it comes
> > > from, and at what exact revision if it comes from a VCS (avoiding tags and
> > > prefering sha1s).
> > 
> > Hm, actually could you please point me to an update script I can use as an
> > example of the sort of script you'd like to see? I see quite a few different
> > approaches in the tree today.
> > 
> > I don't really have strong feelings on the contents of such a script, but I
> > can't imagine it needing to be much more complicated than a clone and
> > copying w/o the git metadata into the third_party/msgpack/ dir (maybe using
> > `git archive`), right?

Oh I missed that you wrote "and/or a readme"... both msgpack and libprio imports are dead simple (hosted on github, we want a particular revision) so I'd prefer to just do it that way... if we had some standard script or (ideally) tool built into mach (maybe this is what `mach vendor` will do someday?) this would be super nice in the future.

Comment 66

[Mike Hommey [:glandium]]

(In reply to Robert Helmer [:rhelmer] from comment #65)
> Oh I missed that you wrote "and/or a readme"... both msgpack and libprio
> imports are dead simple (hosted on github, we want a particular revision) so
> I'd prefer to just do it that way... if we had some standard script or
> (ideally) tool built into mach (maybe this is what `mach vendor` will do
> someday?) this would be super nice in the future.

Bug 1454867

Comment 67

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review268332

> This looks much better than what I am doing! I am totally fine returning a pair of `ByteString`s instead of a pair of `Array`s.
> 
> However if I try to use `.Adopt` here then compilation fails with:
>   error: no member named 'Adopt' in 'mozilla::dom::Optional<nsTString<char> >'
>   
> I don't feel like I have a good handle on the different string types and how ByteString fits into this :/ I've left the code commented out so hopefully you'll see what I mean :)

Oh, the problem is right there.. I copy/pasted that error message without seeing the `mozilla::dom::Optional` bit. I'll see if I can get the rest of it working now that I have that fixed.

Comment 68

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review268332

> Unicode isn't important here.
> 
> So approach 1 lgtm works with `DOMString`, but if I try it with `ByteString` compile fails with:
>   error: no viable conversion from 'const nsTString<char>' to 'const nsAString' (aka 'const nsTSubstring<char16_t>')
>   
> I've left a FIXME in the WebIDL if you want to try it :)

Oh, maybe it'd help if I changed the string type in the Encode function signature...

Comment 69

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/6-7/

Comment 70

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/7-8/

Comment 71

[Gregory Szorc [:gps]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

https://reviewboard.mozilla.org/r/261618/#review269188

Cancelling review because glandium is a build peer and since he is reviewing the vendoring of msgpack, it makes sense for him to review the build system integration too.

That being said, this patch is missing the referenced third_party/msgpack/moz.build file. The build system will reject the patch as-is.

Comment 72

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

https://reviewboard.mozilla.org/r/261618/#review269188

Oh thanks for catching that, I accidentally got the moz.build in the "vendoring" commit not the "build integration" commit... will fix and redirect to glandium. Thanks!

Comment 73

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261616/diff/2-3/

Comment 74

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261618/diff/2-3/

Comment 75

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/8-9/

Comment 76

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review268332

> Oh, the problem is right there.. I copy/pasted that error message without seeing the `mozilla::dom::Optional` bit. I'll see if I can get the rest of it working now that I have that fixed.

Hm. Well if I try to do `data.mA.Value()` then the string seems to be uninitialized... I tried using a combination of `Construct()` and then `Adopt()`, not sure I am doing this right :) Please let me know.

Comment 77

[Robert Helmer [:rhelmer]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251950/diff/5-6/

Comment 78

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261616/diff/3-4/

Comment 79

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261618/diff/3-4/

Comment 80

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/9-10/

Comment 81

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8998961 [details]
Bug 1421501 - export NSS [Init,Shutdown]Context symbols

https://reviewboard.mozilla.org/r/261664/#review269208

Comment 82

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

https://reviewboard.mozilla.org/r/251948/#review269206

I'd be fine with this if I get another commit with `rm -rf third_party/prio/mpi` :)
But I guess both of it should go into some vendouring script (clone, copy, rm mpi).

Comment 83

[Robert Helmer [:rhelmer]]

Regarding update scripts for third-party libs, I poked around and https://searchfox.org/mozilla-central/source/gfx/harfbuzz/update.sh seems to be along the lines of what folks are asking for, I'll amen my vendoring patches.

Really looking forward to Bug 1454867 :)

Comment 84

[Robert Helmer [:rhelmer]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251948/diff/5-6/

Comment 85

[Robert Helmer [:rhelmer]]

Comment on attachment 8998961 [details]
Bug 1421501 - export NSS [Init,Shutdown]Context symbols

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261664/diff/1-2/

Comment 86

[Robert Helmer [:rhelmer]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251950/diff/6-7/

Comment 87

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261616/diff/4-5/

Comment 88

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261618/diff/4-5/

Comment 89

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/10-11/

Comment 90

[Robert Helmer [:rhelmer]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251948/diff/6-7/

Comment 91

[Robert Helmer [:rhelmer]]

Comment on attachment 8998961 [details]
Bug 1421501 - export NSS [Init,Shutdown]Context symbols

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261664/diff/2-3/

Comment 92

[Robert Helmer [:rhelmer]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251950/diff/7-8/

Comment 93

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261616/diff/5-6/

Comment 94

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261618/diff/5-6/

Comment 95

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/11-12/

Comment 96

[Henry Corrigan-Gibbs [:henrycg]]

@rhelmer: Is there any way to make PrioEncoder.encode() return a pair of Uint8Array's? Returning these weird raw bytestrings seems sort of error-prone, since it's not obvious how to convert them into Uint8Arrays (as you and I experienced).

Comment 97

[Robert Helmer [:rhelmer]]

(In reply to Henry Corrigan-Gibbs from comment #96)
> @rhelmer: Is there any way to make PrioEncoder.encode() return a pair of
> Uint8Array's? Returning these weird raw bytestrings seems sort of
> error-prone, since it's not obvious how to convert them into Uint8Arrays (as
> you and I experienced).

Poking around in the DOM code, I see that TextEncoder declares that encode returns a Uint8Array and the code looking at TextEncoder.cpp I don't see why we couldn't do the same thing.

In the near term, our only real use of this output is to re-encode in some way that can be represented in the Telemetry JSON packet, but I could certainly see a future where we use something more efficient so I don't want to hyper-focus on that use case.

Nika suggested using ByteStrings so I'll defer to her :)

Comment 98

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #97)
> (In reply to Henry Corrigan-Gibbs from comment #96)
> > @rhelmer: Is there any way to make PrioEncoder.encode() return a pair of
> > Uint8Array's? Returning these weird raw bytestrings seems sort of
> > error-prone, since it's not obvious how to convert them into Uint8Arrays (as
> > you and I experienced).
> 
> Poking around in the DOM code, I see that TextEncoder declares that encode
> returns a Uint8Array and the code looking at TextEncoder.cpp I don't see why
> we couldn't do the same thing.
> 
> In the near term, our only real use of this output is to re-encode in some
> way that can be represented in the Telemetry JSON packet, but I could
> certainly see a future where we use something more efficient so I don't want
> to hyper-focus on that use case.
> 
> Nika suggested using ByteStrings so I'll defer to her :)

Although tbqh I'd really like to get this landed so we can get data coming in, and stick with the API we have since it passes tests (both existing Firefox and the libprio tests)... if the Telemetry JS code needs to do a little wrangling to turn bytestrings into typed arrays I think that is something we can easily smooth out later, we definitely are going to need to iterate the API here in order to handle more Telemetry values, anyway.

Comment 99

[Henry Corrigan-Gibbs [:henrycg]]

(In reply to Robert Helmer [:rhelmer] from comment #98)
> (In reply to Robert Helmer [:rhelmer] from comment #97)
> > (In reply to Henry Corrigan-Gibbs from comment #96)
> > > @rhelmer: Is there any way to make PrioEncoder.encode() return a pair of
> > > Uint8Array's? Returning these weird raw bytestrings seems sort of
> > > error-prone, since it's not obvious how to convert them into Uint8Arrays (as
> > > you and I experienced).
> > 
> > Poking around in the DOM code, I see that TextEncoder declares that encode
> > returns a Uint8Array and the code looking at TextEncoder.cpp I don't see why
> > we couldn't do the same thing.
> > 
> > In the near term, our only real use of this output is to re-encode in some
> > way that can be represented in the Telemetry JSON packet, but I could
> > certainly see a future where we use something more efficient so I don't want
> > to hyper-focus on that use case.
> > 
> > Nika suggested using ByteStrings so I'll defer to her :)
> 
> Although tbqh I'd really like to get this landed so we can get data coming
> in, and stick with the API we have since it passes tests (both existing
> Firefox and the libprio tests)...

That sounds like a good plan to me.

Comment 100

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review268332

> We should move the shared state into a StaticAutoPtr struct which is initialized when we need it. We can then skip this code and just use mozilla::ClearOnShutdown (https://searchfox.org/mozilla-central/rev/e52cd92858800a69b74cb97d26d9bdb960d611ca/xpcom/base/ClearOnShutdown.h#96-116) to clear the pointer & free the backing memory when the browser is shutting down.

Oh hrm actually I can't figure out how to actually run all this Prio-specific shutdown when using mozilla::ClearOnShutdown instead of the explicit "xpcom-shutdown" observer I was doing before... having a lot of trouble finding examples in the tree of this.

Comment 101

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

https://reviewboard.mozilla.org/r/251948/#review269330

Thanks! This looks good to me.
I guess the final version that's going to land might change, pulling in a newer revision from github. As long as you make sure that the code is reviewed properly there I'm fine with carrying over the r+ to whatever lands in the end.

Comment 102

[Mike Hommey [:glandium]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

https://reviewboard.mozilla.org/r/261616/#review269500

::: third_party/msgpack/update.sh:11
(Diff revision 6)
> +MY_TEMP_DIR=`mktemp -d -t msgpack_update.XXXXXX` || exit 1
> +
> +VERSION=1.8.7
> +
> +git clone https://github.com/msgpack/msgpack-c ${MY_TEMP_DIR}/msgpack
> +git -C ${MY_TEMP_DIR}/msgpack checkout ${VERSION}

This doesn't work for the version that is included here. Ideally, the script should work for the present code, and running it with no modification should put the tree in the same state as it is.

::: third_party/third_party/msgpack/README-mozilla:1
(Diff revision 6)
> +This directory contains the msgpack source from the upstream repo:

Looks like this file got here by mistake.

Comment 103

[Mike Hommey [:glandium]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

https://reviewboard.mozilla.org/r/261618/#review269502

::: config/external/msgpack/moz.build:7
(Diff revision 6)
> +# vim: set filetype=python:
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DIRS += ['../../../third_party/msgpack']

/third_party/msgpack (without leading ../../..) should work

Comment 104

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

https://reviewboard.mozilla.org/r/261616/#review269500

> Looks like this file got here by mistake.

As above I used `gfx/harfbuzz` and an example, it has a file just like this and the script updates the revision number automatically:

https://searchfox.org/mozilla-central/source/gfx/harfbuzz/README-mozilla

Comment 105

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

https://reviewboard.mozilla.org/r/261616/#review269500

> This doesn't work for the version that is included here. Ideally, the script should work for the present code, and running it with no modification should put the tree in the same state as it is.

Oh nevermind I see what you mean here, we do specify the version that gets passed to `git checkout -r`, I'll fix this.

Still unsure about the README-mozilla though :)

Comment 106

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261616/diff/6-7/

Comment 107

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261618/diff/6-7/

Comment 108

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/12-13/

Comment 109

[Mike Hommey [:glandium]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

https://reviewboard.mozilla.org/r/261616/#review269620

::: .pkg/github.com/serulian/corelib/tag/v0.3.1/.gitignore:1
(Diff revision 7)
> +.pkg

All those files in .pkg are surely not intended to be added...

::: third_party/msgpack/README-mozilla:5
(Diff revision 7)
> +This directory contains the msgpack source from the upstream repo:
> +
> +https://github.com/msgpack/msgpack-c
> +
> +Current version: 3a615bcf44ee23ce2e2c94aad73b7ff74374df3c [commit 2c4f2b890ef1546fc022d270d11e657f6fc8022f]

Something obviously went wrong here.

::: third_party/third_party/msgpack/README-mozilla:1
(Diff revision 7)
> +This directory contains the msgpack source from the upstream repo:

This file still shouldn't be there. Note its path.

Comment 110

[Edgar Chen [:edgar]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review269650

::: dom/prio/PrioEncoder.cpp:25
(Diff revision 13)
> +
> +PrioEncoder::PrioEncoder() = default;
> +PrioEncoder::~PrioEncoder()
> +{
> +  if (sPublicKeyA) {
> +    PublicKey_clear (sPublicKeyA);

Nit: remove the space before `(`. And other place for PublicKey_clear.

::: dom/prio/PrioEncoder.cpp:27
(Diff revision 13)
> +PrioEncoder::~PrioEncoder()
> +{
> +  if (sPublicKeyA) {
> +    PublicKey_clear (sPublicKeyA);
> +  }
> +  sPublicKeyA = nullptr;

Put this in above if block.

::: dom/prio/PrioEncoder.cpp:32
(Diff revision 13)
> +  sPublicKeyA = nullptr;
> +
> +  if (sPublicKeyB) {
> +    PublicKey_clear (sPublicKeyB);
> +  }
> +  sPublicKeyB = nullptr;

Ditto.

::: dom/prio/PrioEncoder.cpp:60
(Diff revision 13)
> +  SECStatus prio_rv = SECSuccess;
> +
> +  if (!sSingleton) {
> +    ClearOnShutdown(&sSingleton);
> +
> +    Prio_init();

I don't see sSingleton be initialized in any place, so the cleanup in destructor won't be executed on shutdown. And we will enter this if-block every time we call Encode which will do Prio_init() multiple times, I guess this is not what we expect. Or did I miss something? :)

::: dom/prio/PrioEncoder.cpp:87
(Diff revision 13)
> +        || !PrioEncoder::IsValidHexPublicKey(prefStringKeyB.get(), prefStringKeyB.Length()))  {
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +
> +    if (sPublicKeyA) PublicKey_clear (sPublicKeyA);

if (sPublicKeyA) {
  PublicKey_clear(sPublicKeyA);
}

But I am not sure why we need this, it seems sPublicKeyA should be null at this point if we initialize sSingleton at correct time.

::: dom/prio/PrioEncoder.cpp:149
(Diff revision 13)
> +
> +  return promise.forget();
> +}
> +
> +bool
> +PrioEncoder::IsValidHexPublicKey(const char *str, size_t strlen)

* goes with type, i.e. `const char* str`.

::: dom/prio/PrioEncoder.cpp:155
(Diff revision 13)
> +{
> +  if (strlen != CURVE25519_KEY_LEN_HEX) {
> +    return false;
> +  }
> +
> +  for (size_t i=0; i < strlen; i++) {

Nit: add spaces around `=`.

Comment 111

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/13-14/

Comment 112

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

https://reviewboard.mozilla.org/r/261616/#review269620

> Something obviously went wrong here.

Hrm, this seems to happen when I use a tag for the version in the update script, which is weird:
VERSION="cpp-3.0.1"

I'll fix it up in any case.

> This file still shouldn't be there. Note its path.

Ugh, thanks for pointing this out, that is what I was missing when you pointed this out before.

Comment 113

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Unmarking myself as I'm quite busy. :echen has taken over DOM reviewing

Comment 114

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261616/diff/7-8/

Comment 115

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261618/diff/7-8/

Comment 116

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/14-15/

Comment 117

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

https://reviewboard.mozilla.org/r/261616/#review269620

> All those files in .pkg are surely not intended to be added...

Ugh, this is thanks to the `hg addremove` in `update.sh` ... sorry about that, I am going to leave this approach in the update.sh since this is what the others use.

Comment 118

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/15-16/

Comment 119

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/16-17/

Comment 120

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

One of these days I will learn how to bugzilla... sorry for the spam.

Comment 121

[Mike Hommey [:glandium]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

https://reviewboard.mozilla.org/r/261616/#review269664

::: third_party/msgpack/update.sh:10
(Diff revision 8)
> +
> +MY_TEMP_DIR=`mktemp -d -t msgpack_update.XXXXXX` || exit 1
> +
> +VERSION="2c4f2b890ef1546fc022d270d11e657f6fc8022f"
> +
> +git clone https://github.com/msgpack/msgpack-c ${MY_TEMP_DIR}/msgpack

git clone -n ...

::: third_party/msgpack/update.sh:14
(Diff revision 8)
> +
> +git clone https://github.com/msgpack/msgpack-c ${MY_TEMP_DIR}/msgpack
> +git -C ${MY_TEMP_DIR}/msgpack checkout ${VERSION}
> +
> +COMMIT=$(git -C ${MY_TEMP_DIR}/msgpack rev-parse HEAD)
> +perl -p -i -e "s/(\d+\.)(\d+\.)(\d+)/${VERSION}/" README-mozilla;

So the problem here is that the README-mozilla file doesn't current contain a x.y.z version string, so it's never going to actually do this.

You might as well do it in one go and without relying on the format with something like:
perl -p -i -e "s/Current version: \s+ \[commit [0-9a-f]{40}\]/Current version: ${VERSION} [commit ${COMMIT}]/"

It would also be better to set COMMIT to 2c4f2b... and then set VERSION from `git describe --tags`.

::: third_party/msgpack/update.sh:27
(Diff revision 8)
> +done
> +
> +rm -rf ${MY_TEMP_DIR}
> +
> +hg revert -r . moz.build
> +hg addremove

You can limit the damage by making this do `hg addremove .` instead of `hg addremove`.

Comment 122

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261616/diff/8-9/

Comment 123

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261618/diff/8-9/

Comment 124

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/17-18/

Comment 125

[Henri Sivonen (:hsivonen)]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review269668

::: dom/chrome-webidl/PrioEncoder.webidl:9
(Diff revision 18)
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[ChromeOnly, Exposed=(Window,System)]
> +namespace PrioEncoder {
> +  readonly attribute DOMString version;

What's the use case for this field?

::: dom/prio/PrioEncoder.h:44
(Diff revision 18)
> +
> +  static StaticRefPtr<PrioEncoder>
> +  sSingleton;
> +
> +  static bool
> +  IsValidHexPublicKey(const char *str, size_t strlen);

Please make this method take `mozilla::Span<const char>`. (The modern way to handle a pointer and a length.)

::: dom/prio/PrioEncoder.cpp:41
(Diff revision 18)
> +
> +/* static */ void
> +PrioEncoder::GetVersion(GlobalObject& aGlobal, nsString& aVersion)
> +{
> +  // TODO expose version number from vendored libprio
> +  aVersion = NS_LITERAL_STRING("1.0");

`aVersion.AssignLiteral("1.0");`

::: dom/prio/PrioEncoder.cpp:64
(Diff revision 18)
> +
> +    ClearOnShutdown(&sSingleton);
> +
> +    Prio_init();
> +
> +    nsAutoString prioKeyA;

Please declare this as `nsAutoCStringN<CURVE25519_KEY_LEN_HEX + 1>` and the use `Preferences::GetCString`. (It's unclear to me if `Preferences::GetCString` ever benefits from using an autostring, but if it does, the default is one code unit too short.)

::: dom/prio/PrioEncoder.cpp:71
(Diff revision 18)
> +    if (NS_FAILED(rv)) {
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +
> +    nsAutoString prioKeyB;

Same here.

::: dom/prio/PrioEncoder.cpp:78
(Diff revision 18)
> +    if (NS_FAILED(rv)) {
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +
> +    NS_ConvertUTF16toUTF8 prefStringKeyA(prioKeyA);

Then these conversions are unnecessary.

::: dom/prio/PrioEncoder.cpp:83
(Diff revision 18)
> +    NS_ConvertUTF16toUTF8 prefStringKeyA(prioKeyA);
> +    NS_ConvertUTF16toUTF8 prefStringKeyB(prioKeyB);
> +
> +    // Check that both public keys are of the right length
> +    // and contain only hex digits 0-9a-fA-f
> +    if (!PrioEncoder::IsValidHexPublicKey(prefStringKeyA.get(), prefStringKeyA.Length())

Once `IsValidHexPublicKey` takes a `mozilla::Span<const char>`, you can pass an `nsACString` directly without manually extracting the pointer and the length.

::: dom/prio/PrioEncoder.cpp:89
(Diff revision 18)
> +        || !PrioEncoder::IsValidHexPublicKey(prefStringKeyB.get(), prefStringKeyB.Length()))  {
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +
> +    prio_rv = PublicKey_import_hex(&sPublicKeyA, (const uint8_t*)prefStringKeyA.get(), CURVE25519_KEY_LEN_HEX);

For the second argument:

1. Please use `reinterpret_cast`.
2. Please use `BeginReading()` instead of `get()`. (Both return the same pointer, but the former indicates to the reader that we don't care about zero-termination while the latter says we do care. Here we don't.)
3. Please cast to `const unsigned char*`, since that's what `PublicKey_import_hex` is declared to take. (`uint8_t` is the same as `unsigned char` currently, but the C++ spec doesn't guarantee that.)

::: dom/prio/PrioEncoder.cpp:95
(Diff revision 18)
> +    if (prio_rv != SECSuccess) {
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> +      return nullptr;
> +    }
> +
> +    prio_rv = PublicKey_import_hex(&sPublicKeyB, (const uint8_t*)prefStringKeyB.get(), CURVE25519_KEY_LEN_HEX);

Again here.

::: dom/prio/PrioEncoder.cpp:110
(Diff revision 18)
> +    aPrioParams.mStartupCrashDetected,
> +    aPrioParams.mSafeModeUsage,
> +    aPrioParams.mBrowserIsUserDefault
> +  };
> +
> +  PrioConfig prioConfig = PrioConfig_new(mozilla::ArrayLength(dataItems), sPublicKeyA, sPublicKeyB, (const uint8_t*)aBatchID.BeginReading(), aBatchID.Length());

Same thing about pointer casting here.

::: dom/prio/PrioEncoder.cpp:117
(Diff revision 18)
> +  if (!prioConfig) {
> +    promise->MaybeReject(NS_ERROR_FAILURE);
> +    return promise.forget();
> +  }
> +
> +  uint8_t* forServerA = nullptr;

`unsigned char*`, since that's what `PrioClient_encode` takes.

::: dom/prio/PrioEncoder.cpp:118
(Diff revision 18)
> +    promise->MaybeReject(NS_ERROR_FAILURE);
> +    return promise.forget();
> +  }
> +
> +  uint8_t* forServerA = nullptr;
> +  uint32_t lenA = 0;

`unsigned int`, since that's what `PrioClient_encode` takes a pointer to.

::: dom/prio/PrioEncoder.cpp:119
(Diff revision 18)
> +    return promise.forget();
> +  }
> +
> +  uint8_t* forServerA = nullptr;
> +  uint32_t lenA = 0;
> +  uint8_t* forServerB = nullptr;

`unsigned char*`

::: dom/prio/PrioEncoder.cpp:120
(Diff revision 18)
> +  }
> +
> +  uint8_t* forServerA = nullptr;
> +  uint32_t lenA = 0;
> +  uint8_t* forServerB = nullptr;
> +  uint32_t lenB = 0;

`unsigned int`

::: dom/prio/PrioEncoder.cpp:127
(Diff revision 18)
> +  prio_rv = PrioClient_encode(prioConfig, dataItems, &forServerA, &lenA, &forServerB, &lenB);
> +
> +  // Package the data into the dictionary
> +  PrioEncodedData data;
> +
> +  data.mA.Construct(nsCString()).Adopt((char*)forServerA, lenA);

Why is an argument to `Construct` needed?

`Adopt` requires the buffer to be zero-terminated, but `PrioClient_encode` does not zero-terminate. Please use `Assign(reinterpret_cast<char*>(forServerA), lenA)` to copy the data and then manually free `forServerA` with whatever is the right way to free memory allocated by `PrioClient_encode`.

(It's sad that our C++ strings prioritize zero-termination for legacy reasons.)

::: dom/prio/PrioEncoder.cpp:149
(Diff revision 18)
> +
> +  return promise.forget();
> +}
> +
> +bool
> +PrioEncoder::IsValidHexPublicKey(const char* str, size_t strlen)

`mozilla::Span<const char> aStr`

::: dom/prio/PrioEncoder.cpp:151
(Diff revision 18)
> +}
> +
> +bool
> +PrioEncoder::IsValidHexPublicKey(const char* str, size_t strlen)
> +{
> +  if (strlen != CURVE25519_KEY_LEN_HEX) {

`aStr.Length() != CURVE25519_KEY_LEN_HEX`

::: dom/prio/PrioEncoder.cpp:155
(Diff revision 18)
> +{
> +  if (strlen != CURVE25519_KEY_LEN_HEX) {
> +    return false;
> +  }
> +
> +  for (size_t i = 0; i < strlen; i++) {

Please use a C++ range-based loop to loop over the span.

::: dom/prio/PrioEncoder.cpp:156
(Diff revision 18)
> +  if (strlen != CURVE25519_KEY_LEN_HEX) {
> +    return false;
> +  }
> +
> +  for (size_t i = 0; i < strlen; i++) {
> +    if (!isxdigit(str[i])) {

Please don't use `isxdigit` from the C standard library, because it's not guaranteed to be limited to ASCII. Instead, please add a function to `mfbt/TextUtils.h` to do the specific ASCII-only thing.

Comment 126

[Henri Sivonen (:hsivonen)]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review269670

::: dom/prio/PrioEncoder.cpp:41
(Diff revision 18)
> +
> +/* static */ void
> +PrioEncoder::GetVersion(GlobalObject& aGlobal, nsString& aVersion)
> +{
> +  // TODO expose version number from vendored libprio
> +  aVersion = NS_LITERAL_STRING("1.0");

`aVersion.AssignLiteral(u"1.0");` is even better.

Comment 127

[Henri Sivonen (:hsivonen)]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review269678

::: dom/chrome-webidl/PrioEncoder.webidl:22
(Diff revision 18)
> +  required boolean safeModeUsage;
> +  required boolean browserIsUserDefault;
> +};
> +
> +dictionary PrioEncodedData {
> +  ByteString a;

`ByteString` is for stuff like HTTP headers: things that are represented to JS as UTF-16 strings but that convert from bytes by zero-extending. It seems that these should be `Uint8Array` instead.

Comment 128

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/18-19/

Comment 129

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review269650

> I don't see sSingleton be initialized in any place, so the cleanup in destructor won't be executed on shutdown. And we will enter this if-block every time we call Encode which will do Prio_init() multiple times, I guess this is not what we expect. Or did I miss something? :)

Oh, ugh. No wonder the cleanup wasn't being run (per my reply to the open issue from Nika on this bug :)) I just tested and now we only call the constructor once on first use and destructor gets called once on app shutdown.

> if (sPublicKeyA) {
>   PublicKey_clear(sPublicKeyA);
> }
> 
> But I am not sure why we need this, it seems sPublicKeyA should be null at this point if we initialize sSingleton at correct time.

I think this was a holdover from a previous approach and that you are correct, removed.

Comment 130

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review269678

> `ByteString` is for stuff like HTTP headers: things that are represented to JS as UTF-16 strings but that convert from bytes by zero-extending. It seems that these should be `Uint8Array` instead.

Hm, so the suggestion to use ByteString came up in the previous review... I am fine changing this to `Uint8Array` (this would make our JS code easier in fact) but after changing `PrioEncodedData` dictionary in the WebIDL, I can't figure out how I can create a new `Uint8Array` to the members of that dictionary (`data.mA` and `data.mB`) and can't find great examples in the code of how this should work... are there any pointers you could give here?

Comment 131

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review269668

> What's the use case for this field?

The idea is this is for our own use, for instance if Prio introduces breaking changes in both the client/server in some new version and we need to migrate. If you feel like it's uneccessary I am totally willing to entertain this :)

Comment 132

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #130)
> Comment on attachment 8986638 [details]
> Bug 1421501 - WebIDL and DOM for PrioEncoder
> 
> https://reviewboard.mozilla.org/r/251952/#review269678
> 
> > `ByteString` is for stuff like HTTP headers: things that are represented to JS as UTF-16 strings but that convert from bytes by zero-extending. It seems that these should be `Uint8Array` instead.
> 
> Hm, so the suggestion to use ByteString came up in the previous review... I
> am fine changing this to `Uint8Array` (this would make our JS code easier in
> fact) but after changing `PrioEncodedData` dictionary in the WebIDL, I can't
> figure out how I can create a new `Uint8Array` to the members of that
> dictionary (`data.mA` and `data.mB`) and can't find great examples in the
> code of how this should work... are there any pointers you could give here?

Hm so I am a bit stuck on this one... we have an easy workaround in JS for this in bug 1465251 and we were hoping to revisit later (the idea of returning a Uint8Array directly was brought up in that bug, but in this one ByteArray was suggested)

We have some time constraints (ask me for details if you're curious) and want to start getting data from Nightly users as soon as we can, so if this is not an easy change I'd like to ask that we consider doing this in a followup.

If it is easy enough, I am all for it but could use help per above :) Thanks!

Comment 133

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #132)
> Hm so I am a bit stuck on this one... we have an easy workaround in JS for
> this in bug 1465251 and we were hoping to revisit later (the idea of
> returning a Uint8Array directly was brought up in that bug, but in this one
> ByteArray was suggested)

I read through TypedArray.h and its callers, and I figured out a way to do this... not familiar enough with this to know if the way I am doing it is safe or could be more concise, I'll go ahead and push this for review and cancel the needinfo.

Comment 134

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/19-20/

Comment 135

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/20-21/

Comment 136

[Henri Sivonen (:hsivonen)]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review269668

> The idea is this is for our own use, for instance if Prio introduces breaking changes in both the client/server in some new version and we need to migrate. If you feel like it's uneccessary I am totally willing to entertain this :)

It seems to me that if we ever introduce breaking changes, we could introduce something that's non-`undefined` after that was `undefined` before, so my preference would be not to introduce this field at this time.

Comment 137

[Henri Sivonen (:hsivonen)]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review269690

r+ provided that the comments are addressed. I'd prefer the version field to be removed, but the r+ applies even if you decide to keep it.

::: dom/prio/PrioEncoder.cpp:125
(Diff revision 21)
> +  prio_rv = PrioClient_encode(prioConfig, dataItems, &forServerA, &lenA, &forServerB, &lenB);
> +
> +  // Package the data into the dictionary
> +  PrioEncodedData data;
> +
> +  nsTArray<uint8_t> forServerA_Array;

Please name this `arrayForServerA` to avoid the need for an underscore.

::: dom/prio/PrioEncoder.cpp:128
(Diff revision 21)
> +  PrioEncodedData data;
> +
> +  nsTArray<uint8_t> forServerA_Array;
> +  nsTArray<uint8_t> forServerB_Array;
> +
> +  for (uint8_t i = 0; i < lenA; i++) {

`uint8_t` as the loop variable looks wrong, but instead of a loop, please use `arrayForServerA.AppendElements(reinterpret_cast<uint8_t*>(forServerA), lenA, fallible);` (and check the return value for allocation failure).

::: dom/prio/PrioEncoder.cpp:155
(Diff revision 21)
> +  }
> +  data.mB.Construct().Init(&valueB.toObject());
> +
> +  if (prio_rv != SECSuccess) {
> +    if (prioConfig) {
> +      PrioConfig_clear(prioConfig);

It seems that the earlier failure returns are missing `PrioConfig_clear`. Please remove the manual calls to `PrioConfig_clear` before each return and instead add
```
auto configGuard = MakeScopeExit([&] {
  PrioConfig_clear(prioConfig);
});
```
after the first null-check for `prioConfig` after its creation.

Comment 138

[Edgar Chen [:edgar]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

https://reviewboard.mozilla.org/r/251952/#review269692

r+ with hsivonen's comment addressed.

Comment 139

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/21-22/

Comment 140

[Mike Hommey [:glandium]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

https://reviewboard.mozilla.org/r/261616/#review269696

::: third_party/msgpack/update.sh:11
(Diff revision 9)
> +MY_TEMP_DIR=`mktemp -d -t msgpack_update.XXXXXX` || exit 1
> +
> +COMMIT="2c4f2b890ef1546fc022d270d11e657f6fc8022f"
> +
> +git clone -n https://github.com/msgpack/msgpack-c ${MY_TEMP_DIR}/msgpack
> +git -C ${MY_TEMP_DIR}/msgpack checkout ${VERSION}

this could be COMMIT instead of VERSION. This only works because 2c4f2b89 is the current head of the repository.

Comment 141

[Mike Hommey [:glandium]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

https://reviewboard.mozilla.org/r/261616/#review269696

> this could be COMMIT instead of VERSION. This only works because 2c4f2b89 is the current head of the repository.

I meant should instead of could.

Comment 142

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261616/diff/9-10/

Comment 143

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261618/diff/9-10/

Comment 144

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/22-23/

Comment 145

[Robert Helmer [:rhelmer]]

(In reply to Julien Vehent [:ulfr] from comment #16)
> For future ref, we had a chat today with rhelmer, mreid, ameihm and I about
> the operational security of this experiments. Notes are at
> https://docs.google.com/document/d/1C7cb1YXbYFkElEDOMFnztni_qrmMerhwpvy-
> hu2c47Y/edit#
> 
> In short: data ops should generate the two keys, store them encrypted in
> conf, and send the public keys to rhelmer. A new job (not yet defined) will
> decrypt PRIO payloads inside the telemetry infrastructure and will need
> access to the private keys.

We're going to do this as part of the Telemetry client patch (see bug 1465251 comment 13)

(In reply to Tom Ritter [:tjr] from comment #17)
> Also for future ref: We are creating a new Security Review process and Rob
> was kind enough to be a guinea pig. The review took place in
> https://docs.google.com/document/d/
> 1ru10QrZo8L7iXKWbx3OehbkmdkljTqCVLsjQlItTrW4/edit#

I've filed an issue to track updates for the new third-party repos in https://github.com/mozilla-services/third-party-library-alert/issues/7

--

Also - note that we do have an integration test for Prio, which currently lives outside of the tree in the upstream repo. It uses xpcshell but is driven by a browser-test C program:
https://github.com/mozilla/libprio/tree/master/browser-test

Henry and I have been running this manually so far, it's a bit involved to move this to proper unit test that runs as part of Firefox CI, so I have filed bug 1485620 to track this.

Comment 146

[Robert Helmer [:rhelmer]]

I've been running this through tryserver and running the browser integration test mentioned in comment 145 as we've been moving along, I have rebased against m-c and doing a final round of these tests now:

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

Comment 147

[Robert Helmer [:rhelmer]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251948/diff/7-8/

Comment 148

[Robert Helmer [:rhelmer]]

Comment on attachment 8998961 [details]
Bug 1421501 - export NSS [Init,Shutdown]Context symbols

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261664/diff/3-4/

Comment 149

[Robert Helmer [:rhelmer]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251950/diff/8-9/

Comment 150

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261616/diff/10-11/

Comment 151

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261618/diff/10-11/

Comment 152

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/23-24/

Comment 153

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c119767aec7b
add vendored libprio from https://github.com/mozilla/libprio r=fkiefer
https://hg.mozilla.org/integration/autoland/rev/772dea1abb7f
export NSS [Init,Shutdown]Context symbols r=fkiefer
https://hg.mozilla.org/integration/autoland/rev/1ee0d35a041f
build system integration for libprio r=gps
https://hg.mozilla.org/integration/autoland/rev/7381597721d2
vendor msgpack from https://github.com/msgpack/msgpack-c r=glandium
https://hg.mozilla.org/integration/autoland/rev/600a005d3613
build integration for vendored msgpack r=glandium
https://hg.mozilla.org/integration/autoland/rev/cae4910806c7
WebIDL and DOM for PrioEncoder r=edgar,hsivonen

Comment 154

[Raul Gurzau (:RaulG)]

Backed out 6 changesets (bug 1421501) for bustages on security/nss/lib/freebl/mpi/mp_comba.c on a CLOSED TREE

Backout link: https://hg.mozilla.org/integration/autoland/rev/eee49d0f606bb7423f9688d2901d76296f505a72

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=cae4910806c7976aed9803f22e730c88f7703390

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=195524261&repo=autoland&lineNumber=6300

Log snippet: 

[task 2018-08-23T14:05:59.867Z] 14:05:59     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang -std=gnu99 --target=i686-linux-gnu -o setupintrarecon.o -c  -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DHAVE_CONFIG_H=vpx_config.h -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/media/libvpx -I/builds/worker/workspace/build/src/obj-firefox/media/libvpx -I/builds/worker/workspace/build/src/media/libvpx/config/linux/ia32 -I/builds/worker/workspace/build/src/media/libvpx/config -I/builds/worker/workspace/build/src/media/libvpx/libvpx -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -march=pentium-m -msse -msse2 -mfpmath=sse -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe -g -O2 -fno-omit-frame-pointer -Wno-sign-compare -Wno-unused-function -Wno-unreachable-code -Wno-unneeded-internal-declaration  -MD -MP -MF .deps/setupintrarecon.o.pp   /builds/worker/workspace/build/src/media/libvpx/libvpx/vp8/common/setupintrarecon.c
[task 2018-08-23T14:05:59.867Z] 14:05:59     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/media/libvpx'
[task 2018-08-23T14:05:59.868Z] 14:05:59     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/media/libvpx'
[task 2018-08-23T14:05:59.868Z] 14:05:59     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/media/libvpx'
[task 2018-08-23T14:05:59.884Z] 14:05:59     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/common'
[task 2018-08-23T14:05:59.884Z] 14:05:59     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ --target=i686-linux-gnu -o serv.o -c  -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DU_COMMON_IMPLEMENTATION -DUCONFIG_NO_TRANSLITERATION -DUCONFIG_NO_REGULAR_EXPRESSIONS -DUCONFIG_NO_LEGACY_CONVERSION -DU_USING_ICU_NAMESPACE=0 -DU_NO_DEFAULT_INCLUDE_UTF_HEADERS=1 -DU_CHARSET_IS_UTF8 -DU_HAVE_NL_LANGINFO_CODESET=0 -I/builds/worker/workspace/build/src/config/external/icu/common -I/builds/worker/workspace/build/src/obj-firefox/config/external/icu/common -I/builds/worker/workspace/build/src/intl/icu/source/i18n -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Wc++1z-compat -Wc++2a-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wno-inline-new-delete -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -Wno-unknown-warning-option -Wno-return-type-c-linkage -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -march=pentium-m -msse -msse2 -mfpmath=sse -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -O2 -fno-omit-frame-pointer -Wno-deprecated-declarations -Wno-type-limits -Wno-unused-but-set-variable -Wno-unused-function -Wno-sign-compare -Wno-maybe-uninitialized -frtti  -MD -MP -MF .deps/serv.o.pp   /builds/worker/workspace/build/src/intl/icu/source/common/serv.cpp
[task 2018-08-23T14:05:59.884Z] 14:05:59     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/common'
[task 2018-08-23T14:05:59.905Z] 14:05:59     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/common'
[task 2018-08-23T14:05:59.906Z] 14:05:59     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/common'
[task 2018-08-23T14:05:59.907Z] 14:05:59     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/common'
[task 2018-08-23T14:05:59.907Z] 14:05:59     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/config/external/icu/common'
[task 2018-08-23T14:05:59.934Z] 14:05:59     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/media/libvpx'
[task 2018-08-23T14:05:59.934Z] 14:05:59     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang -std=gnu99 --target=i686-linux-gnu -o swapyv12buffer.o -c  -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DHAVE_CONFIG_H=vpx_config.h -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/media/libvpx -I/builds/worker/workspace/build/src/obj-firefox/media/libvpx -I/builds/worker/workspace/build/src/media/libvpx/config/linux/ia32 -I/builds/worker/workspace/build/src/media/libvpx/config -I/builds/worker/workspace/build/src/media/libvpx/libvpx -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -march=pentium-m -msse -msse2 -mfpmath=sse -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe -g -O2 -fno-omit-frame-pointer -Wno-sign-compare -Wno-unused-function -Wno-unreachable-code -Wno-unneeded-internal-declaration  -MD -MP -MF .deps/swapyv12buffer.o.pp   /builds/worker/workspace/build/src/media/libvpx/libvpx/vp8/common/swapyv12buffer.c
[task 2018-08-23T14:05:59.934Z] 14:05:59     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/media/libvpx'
[task 2018-08-23T14:05:59.935Z] 14:05:59     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/third_party/prio'
[task 2018-08-23T14:05:59.936Z] 14:05:59     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang -std=gnu99 --target=i686-linux-gnu -o mp_comba.o -c  -DNDEBUG=1 -DTRIMMED=1 -DPRIO_BUILD_LIBRARY -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/third_party/prio -I/builds/worker/workspace/build/src/obj-firefox/third_party/prio -I/builds/worker/workspace/build/src/security/nss/lib/freebl/mpi -I/builds/worker/workspace/build/src/third_party/msgpack/include -I/builds/worker/workspace/build/src/third_party/prio/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -DMOZILLA_CLIENT -Qunused-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Qunused-arguments -Wall -Wempty-body -Wignored-qualifiers -Wpointer-arith -Wshadow-field-in-constructor-modified -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wclass-varargs -Wfloat-overflow-conversion -Wfloat-zero-conversion -Wloop-analysis -Werror=non-literal-null-conversion -Wstring-conversion -Wtautological-overlap-compare -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -march=pentium-m -msse -msse2 -mfpmath=sse -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe -g -O2 -fno-omit-frame-pointer  -MD -MP -MF .deps/mp_comba.o.pp   /builds/worker/workspace/build/src/security/nss/lib/freebl/mpi/mp_comba.c
[task 2018-08-23T14:05:59.936Z] 14:05:59     INFO -  /builds/worker/workspace/build/src/security/nss/lib/freebl/mpi/mp_comba.c:151:5: error: register %rax is only available in 64-bit mode
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -      MULADD(at[0], at[4]);
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -      ^
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -  /builds/worker/workspace/build/src/security/nss/lib/freebl/mpi/mp_comba.c:53:9: note: expanded from macro 'MULADD'
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -          "movq  %6,%%rax     \n\t"                   \
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -          ^
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -  <inline asm>:1:18: note: instantiated into assembly here
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -          movq  312(%esp),%rax
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -                          ^~~~
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -  /builds/worker/workspace/build/src/security/nss/lib/freebl/mpi/mp_comba.c:151:5: error: instruction requires: 64-bit mode
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -      MULADD(at[0], at[4]);
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -      ^
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -  /builds/worker/workspace/build/src/security/nss/lib/freebl/mpi/mp_comba.c:53:31: note: expanded from macro 'MULADD'
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -          "movq  %6,%%rax     \n\t"                   \
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -                                ^
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -  <inline asm>:2:2: note: instantiated into assembly here
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -          mulq  304(%esp)
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -          ^
[task 2018-08-23T14:05:59.943Z] 14:05:59     INFO -  /builds/worker/workspace/build/src/security/nss/lib/freebl/mpi/mp_comba.c:151:5: error: register %rax is only available in 64-bit mode

Comment 155

[Robert Helmer [:rhelmer]]

Hm, getting this crash for Linux32 and Android:

https://treeherder.mozilla.org/logviewer.html#?job_id=195524261&repo=autoland&lineNumber=6300

I'm pretty sure that this file isn't actually used by libprio and is erroneously in the libprio build script, so got copied to our moz.build

I'll do a small followup fix.

Comment 156

[Robert Helmer [:rhelmer]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251950/diff/9-10/

Comment 157

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261616/diff/11-12/

Comment 158

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261618/diff/11-12/

Comment 159

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/24-25/

Comment 160

[Robert Helmer [:rhelmer]]

Try run incl. 32-bit platforms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26226ad33601b0b18d46d86a22d5c6b9a2f94c6d

Comment 161

[Robert Helmer [:rhelmer]]

Hm, looks like libprio doesn't build on android (which is fine I'll ifdef it off for now and file an issue on the upstream repo and we can spend some time digging into it):

https://treeherder.mozilla.org/logviewer.html#?job_id=195528704&repo=try

Comment 162

[Robert Helmer [:rhelmer]]

Attachment: Bug 1421501 - disable PrioEncoder on Android until libprio supports it

This is tracked upstream at https://github.com/mozilla/libprio/issues/15

Review commit: https://reviewboard.mozilla.org/r/261714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261714/

Comment 163

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #161)
> Hm, looks like libprio doesn't build on android (which is fine I'll ifdef it
> off for now and file an issue on the upstream repo and we can spend some
> time digging into it):
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=195528704&repo=try

OK pushed a new patch for the above, looks like it's working:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=795a0a090a4613ceb19b93f682ddb2f80f2d5b9f

Upstream issue is https://github.com/mozilla/libprio/issues/15

Comment 164

[Nathan Froyd [:froydnj]]

Comment on attachment 9003591 [details]
Bug 1421501 - disable PrioEncoder on Android until libprio supports it

https://reviewboard.mozilla.org/r/261714/#review269702

This all seems fine, with the possible exception of the below.  I think this would be the first instance in tree of conditionally-assigned `BUG_COMPONENT`, and so I'm a bit suspicious.  WDYT?

::: third_party/moz.build:13
(Diff revision 1)
> +if CONFIG['OS_TARGET'] != 'Android':
> -with Files('prio/**'):
> +    with Files('prio/**'):
> -    BUG_COMPONENT = ('Firefox Build System', 'General')
> +        BUG_COMPONENT = ('Firefox Build System', 'General')
>  
> -with Files('msgpack/**'):
> +    with Files('msgpack/**'):
> -    BUG_COMPONENT = ('Firefox Build System', 'General')
> +        BUG_COMPONENT = ('Firefox Build System', 'General')

This seems like a peculiar change: the files still belong to that `BUG_COMPONENT`, regardless of what platform we're compiling for, no?

Comment 165

[Robert Helmer [:rhelmer]]

Comment on attachment 9003591 [details]
Bug 1421501 - disable PrioEncoder on Android until libprio supports it

https://reviewboard.mozilla.org/r/261714/#review269702

Hm good point... I don't think it's going to be much of a problem, we do plan to fix this upstream and get Android support going probably in the next day or so, at which point I'll just back this all out :)

Thanks for the quick review!

Comment 166

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7f9e47f0608
add vendored libprio from https://github.com/mozilla/libprio r=fkiefer
https://hg.mozilla.org/integration/autoland/rev/a6c9888b5179
export NSS [Init,Shutdown]Context symbols r=fkiefer
https://hg.mozilla.org/integration/autoland/rev/e27d93ac79a0
build system integration for libprio r=gps
https://hg.mozilla.org/integration/autoland/rev/4d7c3c02ce8d
vendor msgpack from https://github.com/msgpack/msgpack-c r=glandium
https://hg.mozilla.org/integration/autoland/rev/3affc66728b0
build integration for vendored msgpack r=glandium
https://hg.mozilla.org/integration/autoland/rev/844232d77d0d
WebIDL and DOM for PrioEncoder r=edgar,hsivonen
https://hg.mozilla.org/integration/autoland/rev/492f05d220b1
disable PrioEncoder on Android until libprio supports it r=froydnj

Comment 167

[Robert Helmer [:rhelmer]]

Argh. So turns out this breaks a few of our more esoteric builders too (asan, cross-compiled macOS, NoOpt debug):
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=492f05d220b1495339691c2bf6c74c106a89b867

So, I am going to just go and fix the upstream bug in https://github.com/mozilla/libprio/issues/15 and then re-vendor. I'd rather not have to ifdef off Android, anyway.

Comment 168

[Cosmin Sabou [:CosminS]]

Backed out 7 changesets (bug 1421501) for causing build bustages

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=492f05d220b1495339691c2bf6c74c106a89b867&tochange=6d71f0af2bccbdc3940f32d29f2408bc3dbf1567&filter-searchStr=build&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=195596326

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=195596326&repo=autoland&lineNumber=37089

Backout link: https://hg.mozilla.org/integration/autoland/rev/6d71f0af2bccbdc3940f32d29f2408bc3dbf1567

task 2018-08-23T19:36:58.256Z] 19:36:58     INFO -  /builds/worker/workspace/build/src/third_party/prio/prio/util.h:74: multiple definition of `next_power_of_two'
[task 2018-08-23T19:36:58.256Z] 19:36:58     INFO -  ../../third_party/prio/client.o:/builds/worker/workspace/build/src/third_party/prio/prio/util.h:74: first defined here
[task 2018-08-23T19:36:58.257Z] 19:36:58     INFO -  clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)
[task 2018-08-23T19:36:58.257Z] 19:36:58     INFO -  /builds/worker/workspace/build/src/config/rules.mk:705: recipe for target 'libxul.so' failed
[task 2018-08-23T19:36:58.257Z] 19:36:58     INFO -  make[4]: *** [libxul.so] Error 1
[task 2018-08-23T19:36:58.257Z] 19:36:58     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/library'
[task 2018-08-23T19:36:58.257Z] 19:36:58     INFO -  /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'toolkit/library/target' failed
[task 2018-08-23T19:36:58.257Z] 19:36:58     INFO -  make[3]: *** [toolkit/library/target] Error 2
[task 2018-08-23T19:36:58.257Z] 19:36:58     INFO -  /builds/worker/workspace/build/src/config/recurse.mk:32: recipe for target 'compile' failed
[task 2018-08-23T19:36:58.257Z] 19:36:58     INFO -  make[2]: *** [compile] Error 2
[task 2018-08-23T19:36:58.257Z] 19:36:58     INFO -  /builds/worker/workspace/build/src/config/rules.mk:432: recipe for target 'default' failed
[task 2018-08-23T19:36:58.257Z] 19:36:58     INFO -  make[1]: *** [default] Error 2
[task 2018-08-23T19:36:58.257Z] 19:36:58     INFO -  client.mk:150: recipe for target 'build' failed
[task 2018-08-23T19:36:58.257Z] 19:36:58     INFO -  make: *** [build] Error 2
[task 2018-08-23T19:36:58.305Z] 19:36:58     INFO -  300 compiler warnings present.
[task 2018-08-23T19:36:58.348Z] 19:36:58     INFO -  Notification center failed: Install notify-send (usually part of the libnotify package) to get a notification when the build finishes.
[task 2018-08-23T19:36:58.400Z] 19:36:58    ERROR - Return code: 2
[task 2018-08-23T19:36:58.401Z] 19:36:58  WARNING - setting return code to 2
[task 2018-08-23T19:36:58.401Z] 19:36:58    FATAL - 'mach build -v' did not run successfully. Please check log for errors.
[task 2018-08-23T19:36:58.401Z] 19:36:58    FATAL - Running post_fatal callback...
[task 2018-08-23T19:36:58.401Z] 19:36:58    FATAL - Exiting -1
[task 2018-08-23T19:36:58.401Z] 19:36:58     INFO - [mozharness: 2018-08-23 19:36:58.401813Z] Finished build step (failed)
[task 2018-08-23T19:36:58.401Z] 19:36:58     INFO - Running post-run listener: _summarize
[task 2018-08-23T19:36:58.402Z] 19:36:58    ERROR - # TBPL FAILURE #
[task 2018-08-23T19:36:58.402Z] 19:36:58     INFO - [mozharness: 2018-08-23 19:36:58.402066Z] FxDesktopBuild summary:
[task 2018-08-23T19:36:58.402Z] 19:36:58    ERROR - # TBPL FAILURE #

Comment 169

[Robert Helmer [:rhelmer]]

OK I think this is all fixed upstream. I am re-vendoring libprio to pick up:

https://github.com/mozilla/libprio/pull/16 (Work with noopt and non 64bit platforms (android, asan, noopt debug, etc)
https://github.com/mozilla/libprio/pull/14 (free PK11 slot as this appears to be leaking NSS resources at shutdown)
https://github.com/mozilla/libprio/pull/13 (Fixes to browser-test)

The last two we're not actually hitting yet, but we're going to want them for bug 1465251 and bug 1485620 anyway.

Comment 170

[Robert Helmer [:rhelmer]]

Comment on attachment 8986636 [details]
Bug 1421501 - add vendored libprio from https://github.com/mozilla/libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251948/diff/8-9/

Comment 171

[Robert Helmer [:rhelmer]]

Comment on attachment 8998961 [details]
Bug 1421501 - export NSS [Init,Shutdown]Context symbols

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261664/diff/4-5/

Comment 172

[Robert Helmer [:rhelmer]]

Comment on attachment 8986637 [details]
Bug 1421501 - build system integration for libprio

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251950/diff/10-11/

Comment 173

[Robert Helmer [:rhelmer]]

Comment on attachment 8998386 [details]
Bug 1421501 - vendor msgpack from https://github.com/msgpack/msgpack-c

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261616/diff/12-13/

Comment 174

[Robert Helmer [:rhelmer]]

Comment on attachment 8998387 [details]
Bug 1421501 - build integration for vendored msgpack

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261618/diff/12-13/

Comment 175

[Robert Helmer [:rhelmer]]

Comment on attachment 8986638 [details]
Bug 1421501 - WebIDL and DOM for PrioEncoder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/251952/diff/25-26/

Comment 176

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f91b4d370bb6
add vendored libprio from https://github.com/mozilla/libprio r=fkiefer
https://hg.mozilla.org/integration/autoland/rev/5599309f2879
export NSS [Init,Shutdown]Context symbols r=fkiefer
https://hg.mozilla.org/integration/autoland/rev/22e788de6e25
build system integration for libprio r=gps
https://hg.mozilla.org/integration/autoland/rev/6857282a711c
vendor msgpack from https://github.com/msgpack/msgpack-c r=glandium
https://hg.mozilla.org/integration/autoland/rev/1e6b919293b6
build integration for vendored msgpack r=glandium
https://hg.mozilla.org/integration/autoland/rev/9b41ca132dbc
WebIDL and DOM for PrioEncoder r=edgar,hsivonen

Comment 177

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/f91b4d370bb6
https://hg.mozilla.org/mozilla-central/rev/5599309f2879
https://hg.mozilla.org/mozilla-central/rev/22e788de6e25
https://hg.mozilla.org/mozilla-central/rev/6857282a711c
https://hg.mozilla.org/mozilla-central/rev/1e6b919293b6
https://hg.mozilla.org/mozilla-central/rev/9b41ca132dbc

Comment 178

[(Away)]

Out of curiosity, does any code outside of xul.dll call the msgpack functions? If not, I'd like to propose that we disable MSGPACK_DLLEXPORT for the same reasons as bug 1523033. I'm happy to file a separate bug but wanted to ask about the usage first.

Comment 179

[Robert Helmer [:rhelmer]]

(In reply to David Major [:dmajor] from comment #178)

Out of curiosity, does any code outside of xul.dll call the msgpack functions?

No.

If not, I'd like to propose that we disable MSGPACK_DLLEXPORT for the same reasons as bug 1523033. I'm happy to file a separate bug but wanted to ask about the usage first.

That sounds great, please do.

Comment 180

[(Away)]

(In reply to Robert Helmer [:rhelmer] from comment #179)

(In reply to David Major [:dmajor] from comment #178)

Out of curiosity, does any code outside of xul.dll call the msgpack functions?

No.

If not, I'd like to propose that we disable MSGPACK_DLLEXPORT for the same reasons as bug 1523033. I'm happy to file a separate bug but wanted to ask about the usage first.

That sounds great, please do.

Great, I filed bug 1523312.