1454606 - Preserve the semantics of scalar operations when restoring the persisted state

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Alessio Placitelli [:Dexter]]

When loading persisted scalar measurements in GeckoView, we need to iron out any race that may happen when changing a scalar whose state is being loaded from the persistence file.

For example, we need to fix the following behaviour:

1. Scalar data loading is started;
2. "test" scalar is set to 10;
3. the state of the "test" scalar is loaded off the persistence file, and the value "14" is discarded as the scalar was already set
4. "test" scalar is incremented via "scalarAdd" by 1. Its value is "11"

The semantics there are a bit confusing and should be fixed.

Comment 1

[Jan-Erik Rediger [:janerik]]

## Summary

When loading persisted scalar measurements in GeckoView, races may happen when the application starts setting scalars before the load is finished.
To mitigate the races we will record the operations on a scalar while the persisted scalar measurements are not fully loaded 
and replay back all operations afterwards.

## Motivation

When loading persisted scalar measurements in GeckoView,
we need to iron out any race that may happen when changing a scalar whose state is being loaded from the persistence file.

For example, we need to fix the following behaviour:

1. Scalar data loading is started;
2. "test" scalar is incremented by 10, ending up at 10 (previous value was unset, so 0);
3. the state of the "test" scalar is loaded off the persistence file, and the value "14" is discarded as the scalar was already set
4. "test" scalar is incremented via "scalarAdd" by 1. Its value is "11".

We expect the value to be 25.

To avoid this situation changes to scalars will not be directly applied while the load is still in progress.
Instead we will record the different operations (`add`, `set`, `setMaximum`) per scalar probe into a list and apply them once the load is finished,
indicated by the `internal-telemetry-geckoview-load-complete` event.
This preserves semantics as if the scalar operations only happened after all data was loaded.

The GeckoView API doesn't allow to take snapshots of any probes before the `load-complete` fired, 
so the fact that operations might be applied late can't be observed.

The above example will be turned into this sequence:

1. Scalar data loading is started;
2. "test" scalar is incremented by "10" -> The operation `[test, add, 10]` is recorded into the list.
3. the state of the "test" scalar is loaded off the persistence file, and the value "14" is set.
4. The `load-complete` event fires and the event handler applies the operations from the list
    1. The "test" scalar is inremented by "10", the value is now "24"
5. "test" scalar is incremented via "scalarAdd" by 1. Its value is "25"


## Implementation

First, we track the status of whether we are loading persisted measurements globally in the Telemetry module.
Once the `internal-telemetry-geckoview-load-complete` event is fired and therefore the load completed, we flip this flag.

Meanwhile, for every scalar operation (`add`, `set`, `setMaximum`) we check the global flag.
If no loading is in progress, we immediately apply the operation as intended.
If loading is still in progress, we record the scalar probe id, the operation and the value into a list of pending operations.
(We handle keyed scalars in the same way, in a separate list.)

Once the `internal-telemetry-geckoview-load-complete` event fires, and in the same event handler as the flag-flipping,
we apply all operations from the pending list in one go.
Afterwards scalar operations can be applied as usual.

## Drawbacks

* This adds a bit of complexity to the scalar operations, however it is only needed on GeckoView (which unfortunately we can't detect at compile time, 
  on Desktop and Fennec however we are never in the loading phase, so the "pending operations list" is never filled)
* Slighly different code paths on Desktop/Fennec and GeckoView. As above no loading phase on Desktop/Fennec, so "pending operations list" is never filled, 
  so even if the code is in there it leaves a single always-false check.
* This will be hard to test. 
  We need to somehow force a loading state, modify scalar probes, then trigger the `load-complete` event to check operations are applied.
  This might require more test-only methods on the module.

## Prior Art

We already accumulate operations for IPC and apply all operations in-order only once.
We can reuse the `ScalarAction` type, the recording code (`TelemetryIPCAccumulator::RecordChildScalarAction`)
and code for applying pending actions (`TelemetryScalar::UpdateChildData`).

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Jan-Erik Rediger [:janerik] from comment #1)
> ## Implementation
> 
> First, we track the status of whether we are loading persisted measurements
> globally in the Telemetry module.
> Once the `internal-telemetry-geckoview-load-complete` event is fired and
> therefore the load completed, we flip this flag.
> 
> Meanwhile, for every scalar operation (`add`, `set`, `setMaximum`) we check
> the global flag.
> If no loading is in progress, we immediately apply the operation as intended.
> If loading is still in progress, we record the scalar probe id, the
> operation and the value into a list of pending operations.
> (We handle keyed scalars in the same way, in a separate list.)
> 
> Once the `internal-telemetry-geckoview-load-complete` event fires, and in
> the same event handler as the flag-flipping,
> we apply all operations from the pending list in one go.
> Afterwards scalar operations can be applied as usual.

This proposal looks solid to me. We need to make absolutely certain that the global flag has the correct state on Desktop and Fennec: on these platforms we are not receiving `internal-telemetry-geckoview-load-complete` and may end up never applying scalar changes.

> * This will be hard to test. 
>   We need to somehow force a loading state, modify scalar probes, then
> trigger the `load-complete` event to check operations are applied.
>   This might require more test-only methods on the module.

Yes, this is a bit tricky (similarly to testing other GV stuff on our end). We should probably be smart about this and find a way to somehow expose this stuff to XPCSHELL tests (maybe not in this bug).

Comment 3

[Jan-Erik Rediger [:janerik]]

Another unresolved question:

Do we need to have a fail-safe to stop unbounded memory growth? IPC has it[1]. It's unlikely to happen during normal operations (loading should finish quickly).
If it does happen, what do we do? (discard operations or immediately apply, then keep collecting)

[1]: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/ipc/TelemetryIPCAccumulator.cpp#44-48

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Jan-Erik Rediger [:janerik] from comment #3)
> Another unresolved question:
> 
> Do we need to have a fail-safe to stop unbounded memory growth? IPC has
> it[1]. It's unlikely to happen during normal operations (loading should
> finish quickly).
> If it does happen, what do we do? (discard operations or immediately apply,
> then keep collecting)
> 
> [1]:
> https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/
> ipc/TelemetryIPCAccumulator.cpp#44-48

I think we should probably apply immediately, to salvage what's salvageable :)

* What if we discard? *

We will loose current enqueued operations. If we had data for scalars that had no persistence information, that bit would be lost for no reason.

* What if we apply immediately? *

We will only loose current enqueued data for the scalars that also had persisted information. Loading the persisted data will end up overwriting these scalars. However, we will retain information for the scalars that had no related data already persisted.

Comment 5

[Chris H-C :chutten]

If we reach a high watermark before the loading completes, it might be best to flip the global flag to "no longer loading" when we apply immediately and assume something's gone wrong enough that observers weren't notified when loading completed. (fail safe)

The proposal seems fine in the broad strokes. We should add some reporting about the reporting (akin to the telemetry.discarded.* from the IPC code: https://telemetry.mozilla.org/probe-dictionary/?search=telemetry.discarded ) so we have a chance to notice if there are problems in the field.

Despite not knowing if it's GV at compile-time, we'll at least be able to detect Desktop at compile-time and avoid introducing GV-specific stuff there (as we have so far)

Comment 6

[Jan-Erik Rediger [:janerik]]

I talked to :gfritzsche directly and explained the rough proposal. He had no objections to the general idea, so I will move forward with the proposal, incorporating both your comments. Thank you for that!

Comment 7

[Alessio Placitelli [:Dexter]]

There's another caveat: we should probably delay snapshotting measurements to after any pending scalar operation was replayed. Otherwise we may return inconsistent scalar measurements if some snapshot was requested *before* the persistence file has been fully loaded.

Comment 8

[Jan-Erik Rediger [:janerik]]

What inconsistency could cause this?
The way it works currently: 
  a snapshot is delayed until the "load-complete" event is fired.
  when the event handler is executed, it locks the storage and gets the data. then serializes it.

With my current approach we add:
  on "load-complete" another handler fires
  this handler locks the storage and replays scalar operations
  it unlocks and scalar operations can proceed as usual

Because of the lock, the scalar storage should always be in a consistent state.
The only thing that can happen is that the snapshot requester sees either a state _before_ operations are applied or _after_.

Comment 9

[Alessio Placitelli [:Dexter]]

(In reply to Jan-Erik Rediger [:janerik] from comment #8)
> The only thing that can happen is that the snapshot requester sees either a
> state _before_ operations are applied or _after_.

Yes, this is what I meant: I'm not sure how big of a deal this is though. Thinking of it again, it seems more of an edge case that will need to be documented than something we should fix, IMHO.

Comment 10

[Chris Peterson [:cpeterson]]

Adding whiteboard tag [geckoview:klar:p1] because Alessio says this bug should blocker Klar+GeckoView release.

Comment 11

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1454606 - Record scalar operations and apply operations after load.

In order to preserve semantics of scalar operations, we defer applying
operations on probes until after the persisted measurements are fully loaded.
This is only necessary on GeckoView, as it is the only product doing intermediate persistence of measurements.

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

Comment 12

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1454606 - Test scalar semantics when pending operations are applied.

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

Comment 13

[Alessio Placitelli [:Dexter]]

Comment on attachment 8979885 [details]
Bug 1454606 - Record scalar operations and apply operations after load.

https://reviewboard.mozilla.org/r/246084/#review252136

Thanks Jan-Erik! I took an high level look and I have two main concerns: how does it work with multi-process? I left a longer comment below about that. The second concern is about the documentation. Would you kindly update the GeckoView in-tree docs (as a separate patch) to mention the reason why we need this, the expected behaviour and any relevant edge case?

::: toolkit/components/telemetry/Telemetry.cpp:1298
(Diff revision 1)
>    if (GetCurrentProduct() == SupportedProduct::Geckoview) {
> +    if (XRE_IsParentProcess()) {
> +      nsCOMPtr<nsIObserverService> observerService = mozilla::services::GetObserverService();
> +      if (observerService) {
> +        observerService->AddObserver(sTelemetry, kLoadCompleteTopic, false);
> +        TelemetryScalar::LoadStarted();

I would prefer all this GV specific logic to live within `TelemetryGeckoViewPersistence.cpp`. We could even go as far as directly calling the setup code from [here](https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/toolkit/components/telemetry/geckoview/TelemetryGeckoViewPersistence.cpp#449). Any reason why we shouldn't do that?

::: toolkit/components/telemetry/TelemetryScalar.h:33
(Diff revision 1)
>  namespace TelemetryScalar {
>  
>  void InitializeGlobalState(bool canRecordBase, bool canRecordExtended);
>  void DeInitializeGlobalState();
> +void LoadStarted();
> +void LoadFinished();

I don't think we need to expose `LoadFinished`. Moreover, since these are GV-specific, can we move them at the end of the file near the serialization/deserialization functions? Let's also add one line to describe what they should be used for!

::: toolkit/components/telemetry/TelemetryScalar.cpp:114
(Diff revision 1)
>    static_cast<uint32_t>(mozilla::Telemetry::ScalarID::ScalarCount);
>  
> +// Whether or not the load of persisted scalars is still in progress.
> +// This is never the case on Desktop or Fennec.
> +// Only GeckoView restores persisted scalars.
> +static bool gScalarIsLoading = false;

Can we move this bool around [line 917](https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryScalar.cpp#917) - unless there's a reason for this to be this early in the file? Any particular reason why this should be static (other global bools, below, are not).

Would you kindly also expand on how this is expected to  behave on processes other than the "parent" one?

::: toolkit/components/telemetry/TelemetryScalar.cpp:116
(Diff revision 1)
> +// Whether or not the load of persisted scalars is still in progress.
> +// This is never the case on Desktop or Fennec.
> +// Only GeckoView restores persisted scalars.
> +static bool gScalarIsLoading = false;
> +// This batches scalar accumulations that should be applied once loading finished.
> +StaticAutoPtr<nsTArray<ScalarAction>> gScalarsActions;

Can we move both these arrays around line 917 as well, near the other storages (if that's possible)?

::: toolkit/components/telemetry/TelemetryScalar.cpp:119
(Diff revision 1)
> +static bool gScalarIsLoading = false;
> +// This batches scalar accumulations that should be applied once loading finished.
> +StaticAutoPtr<nsTArray<ScalarAction>> gScalarsActions;
> +StaticAutoPtr<nsTArray<KeyedScalarAction>> gKeyedScalarsActions;
> +
> +const size_t kScalarActionsArrayHighWaterMark = 10000;

Can you add a comment describing what's this about?

::: toolkit/components/telemetry/TelemetryScalar.cpp:230
(Diff revision 1)
>  }
>  
> +bool
> +internal_IsScalarLoading(const StaticMutexAutoLock& lock)
> +{
> +  return gScalarIsLoading;

nit: `gIsDeserializing`?

::: toolkit/components/telemetry/TelemetryScalar.cpp:1374
(Diff revision 1)
> +    if (sr != ScalarResult::Ok) {
> +      MOZ_ASSERT(false, "Unable to convert nsIVariant to mozilla::Variant.");
> +      return sr;
> +    }
> +    internal_RecordScalarAction(
> +      lock,

nit: why are we adding a line break after `lock,`?

::: toolkit/components/telemetry/TelemetryScalar.cpp:1554
(Diff revision 1)
> +    if (sr != ScalarResult::Ok) {
> +      MOZ_ASSERT(false, "Unable to convert nsIVariant to mozilla::Variant.");
> +      return sr;
> +    }
> +    internal_RecordKeyedScalarAction(
> +      lock,

nit: why are we splitting the line here?

::: toolkit/components/telemetry/TelemetryScalar.cpp:2130
(Diff revision 1)
> +void
> +TelemetryScalar::ApplyPendingOperations()
> +{
> +  StaticMutexAutoLock locker(gTelemetryScalarsMutex);
> +  internal_ApplyPendingOperations(locker);
> +}

Can't we simply drop `LoadFinished` and flip `gScalarIsLoading` here?

::: toolkit/components/telemetry/TelemetryScalar.cpp:3157
(Diff revision 1)
>    StaticMutexAutoLock locker(gTelemetryScalarsMutex);
>    gScalarStorageMap.Clear();
>    gKeyedScalarStorageMap.Clear();
>    gDynamicBuiltinScalarStorageMap.Clear();
>    gDynamicBuiltinKeyedScalarStorageMap.Clear();
> +  if (gScalarsActions) {

We can simply do `gScalarsActions = nullptr;` instead

::: toolkit/components/telemetry/TelemetryScalar.cpp:3160
(Diff revision 1)
>    gDynamicBuiltinScalarStorageMap.Clear();
>    gDynamicBuiltinKeyedScalarStorageMap.Clear();
> +  if (gScalarsActions) {
> +    gScalarsActions->Clear();
> +  }
> +  if (gKeyedScalarsActions) {

Same as above

::: toolkit/components/telemetry/TelemetryScalar.cpp:3208
(Diff revision 1)
> -    if (upd.mData.isNothing()) {
> -      MOZ_ASSERT(false, "There is no data in the ScalarActionType.");
> -      continue;
> -    }
>  
> -    // Get the type of this scalar from the scalar ID. We already checked
> +  internal_ApplyScalarActions(locker, aProcessType, aScalarActions);

Mh, what happens if scalars are still being deserialized but some other process sets scalars and this function gets called? I think this would behave like the `force` mode: it will set the scalars regardless of `gScalarIsLoading`. Is this reasoning correct?

::: toolkit/components/telemetry/ipc/TelemetryComms.h:54
(Diff revision 1)
>    ScalarActionType mActionType;
>    // We need to wrap mData in a Maybe otherwise the IPC system
>    // is unable to instantiate a ScalarAction.
>    Maybe<ScalarVariant> mData;
> +  // The process type this scalar should be recorded for
> +  // or Nothing if determined by the IPC system.

nit: let's mention why this is needed, as the IPC code only uses `Nothing()` for this

Comment 14

[Alessio Placitelli [:Dexter]]

Comment on attachment 8979886 [details]
Bug 1454606 - Test scalar semantics when pending operations are applied.

https://reviewboard.mozilla.org/r/246086/#review252192

I took a first stab at the tests, but I think I should revisit after the changes to the first changeset. Unfortunately, we can't test multi-process in gtests: we'll need xpcshell for that (sigh :'( ). I'll drop a comment in bug 1461965 mentioning that we'll need to add coverage for this. Meanwhile, clearing the review request :)

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:477
(Diff revision 1)
> +  TelemetryScalar::ApplyPendingOperations();
> +  TelemetryScalar::LoadFinished();
> +
> +  // Serialize scalars so we can test them
> +  nsCString json;
> +  JSONWriter jw{ MakeUnique<StringWriteFunc>(json) };

Instead of relying on `JSONWriter`, why don't we check that the snapshot is empty?

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:494
(Diff revision 1)
> +  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, initialValue);
> +
> +  // Force loading mode
> +  TelemetryScalar::LoadStarted();
> +
> +  // Add to a scalar

We're not adding here, but rather setting. Is this intended? I would like to see an addition here rather than a set.

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:541
(Diff revision 1)
> +  TelemetryGeckoViewPersistence::InitPersistence();
> +  loadingFinished->WaitForNotification();
> +
> +  // Force pending operations to be applied and end load mode.
> +  // On GeckoView this happens through an observer on the topic as added by TelemetryImpl on instantiation
> +  TelemetryScalar::ApplyPendingOperations();

Shouldn't this be happening already? The topic is already being notified.

::: toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp:548
(Diff revision 1)
> +
> +  // Increment again, now directly applied
> +  uint32_t val = 1;
> +  Telemetry::ScalarAdd(Telemetry::ScalarID::TELEMETRY_TEST_UNSIGNED_INT_KIND, val);
> +
> +

nit: double empty line

Comment 15

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979885 [details]
Bug 1454606 - Record scalar operations and apply operations after load.

https://reviewboard.mozilla.org/r/246084/#review252136

> We can simply do `gScalarsActions = nullptr;` instead

But this will not call the destructor and therefore won't free objects, right? (Then again, by the time this is called it should be empty).
This only affects tests, shouldn't we still care about cleaning up used memory?

Comment 16

[Alessio Placitelli [:Dexter]]

Comment on attachment 8979885 [details]
Bug 1454606 - Record scalar operations and apply operations after load.

https://reviewboard.mozilla.org/r/246084/#review252136

> But this will not call the destructor and therefore won't free objects, right? (Then again, by the time this is called it should be empty).
> This only affects tests, shouldn't we still care about cleaning up used memory?

This will call the `StaticAutoPtr::operator=` which, in turn, will free the current data and assign the new pointer (`nullptr`). See [here](https://searchfox.org/mozilla-central/rev/bf4def01bf8f6ff0d18f02f2d7e9efc73e12c63f/xpcom/base/StaticPtr.h#62,99).

Comment 17

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979885 [details]
Bug 1454606 - Record scalar operations and apply operations after load.

https://reviewboard.mozilla.org/r/246084/#review252136

> This will call the `StaticAutoPtr::operator=` which, in turn, will free the current data and assign the new pointer (`nullptr`). See [here](https://searchfox.org/mozilla-central/rev/bf4def01bf8f6ff0d18f02f2d7e9efc73e12c63f/xpcom/base/StaticPtr.h#62,99).

ETOOMANYOPERATORSINCPP. Will use this then.

Comment 18

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979885 [details]
Bug 1454606 - Record scalar operations and apply operations after load.

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

Comment 19

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979886 [details]
Bug 1454606 - Test scalar semantics when pending operations are applied.

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

Comment 20

[Alessio Placitelli [:Dexter]]

Comment on attachment 8979885 [details]
Bug 1454606 - Record scalar operations and apply operations after load.

https://reviewboard.mozilla.org/r/246084/#review253302

This looks good with the changes below: good job!

::: toolkit/components/telemetry/TelemetryScalar.h:32
(Diff revision 2)
>  
>  namespace TelemetryScalar {
>  
>  void InitializeGlobalState(bool canRecordBase, bool canRecordExtended);
>  void DeInitializeGlobalState();
> +void DeserializationStarted();

nit: woul you mind moving these below, near the other GeckoView specific functions?

::: toolkit/components/telemetry/TelemetryScalar.cpp:1282
(Diff revision 2)
> + *
> + * @param aScalarAction The action to record.
> + */
> +void
> +internal_RecordScalarAction(const StaticMutexAutoLock& lock,
> +                            ScalarAction aScalarAction)

Should this be a const reference?

::: toolkit/components/telemetry/TelemetryScalar.cpp:1334
(Diff revision 2)
> + *
> + * @param aScalarAction The action to record.
> + */
> +void
> +internal_RecordKeyedScalarAction(const StaticMutexAutoLock& lock,
> +                                 KeyedScalarAction aScalarAction)

Should this be a const reference?

::: toolkit/components/telemetry/TelemetryScalar.cpp:3278
(Diff revision 2)
> -          break;
> -        }
> -      default:
> -        NS_WARNING("Unsupported action coming from scalar child updates.");
>      }
> +  } else {

Let's add a return in the previous `if` clause and drop this `else` and take the `internal_ApplyScalarActions(...)` call out. This would make our [style guide](https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices) happy!

::: toolkit/components/telemetry/TelemetryScalar.cpp:3301
(Diff revision 2)
> -          break;
> -        }
> -      default:
> -        NS_WARNING("Unsupported action coming from keyed scalar child updates.");
>      }
> +  } else {

As above, let's add a `return` in the previous `if` clause, drop the `else` and move the function call out of the `{}`.

::: toolkit/components/telemetry/geckoview/TelemetryGeckoViewPersistence.cpp:517
(Diff revision 2)
>      return;
>    }
>  
>    gPersistenceThread = thread.forget();
>  
> +  // From now on all scalar operations should be recorded

super-nit: trailing '.'

Comment 21

[Alessio Placitelli [:Dexter]]

Comment on attachment 8979886 [details]
Bug 1454606 - Test scalar semantics when pending operations are applied.

https://reviewboard.mozilla.org/r/246086/#review253346

This is some nice and thorough test coverage, I like it. Good job!

Comment 22

[Alessio Placitelli [:Dexter]]

Jan-Erik, are you planning to update the docs in a separate changeset?

Comment 23

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1454606 - Document scalar semantics during persistence deserialization.

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

Comment 24

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979885 [details]
Bug 1454606 - Record scalar operations and apply operations after load.

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

Comment 25

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979886 [details]
Bug 1454606 - Test scalar semantics when pending operations are applied.

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

Comment 26

[Alessio Placitelli [:Dexter]]

Comment on attachment 8981382 [details]
Bug 1454606 - Document scalar semantics during persistence deserialization.

https://reviewboard.mozilla.org/r/247508/#review253566

Comment 27

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e297f97f841b81b77704303f985897f9ce5c55b0 -d d52331db3c62: rebasing 465524:e297f97f841b "Bug 1454606 - Record scalar operations and apply operations after load. r=Dexter"
merging toolkit/components/telemetry/geckoview/TelemetryGeckoViewPersistence.cpp
rebasing 465525:f19a780f7265 "Bug 1454606 - Test scalar semantics when pending operations are applied. r=Dexter"
merging toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp
warning: conflicts while merging toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 28

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979885 [details]
Bug 1454606 - Record scalar operations and apply operations after load.

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

Comment 29

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979886 [details]
Bug 1454606 - Test scalar semantics when pending operations are applied.

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

Comment 30

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8981382 [details]
Bug 1454606 - Document scalar semantics during persistence deserialization.

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

Comment 31

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 30dd381a7c748e18623874819d2c6fc3d67a8b2a -d 85899a77a390: rebasing 465545:30dd381a7c74 "Bug 1454606 - Record scalar operations and apply operations after load. r=Dexter"
merging toolkit/components/telemetry/geckoview/TelemetryGeckoViewPersistence.cpp
rebasing 465546:fc7c7e6b12f7 "Bug 1454606 - Test scalar semantics when pending operations are applied. r=Dexter"
merging toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp
warning: conflicts while merging toolkit/components/telemetry/geckoview/gtest/TestGeckoView.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 32

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979885 [details]
Bug 1454606 - Record scalar operations and apply operations after load.

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

Comment 33

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979886 [details]
Bug 1454606 - Test scalar semantics when pending operations are applied.

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

Comment 34

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8981382 [details]
Bug 1454606 - Document scalar semantics during persistence deserialization.

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

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2889aa95e455
Record scalar operations and apply operations after load. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/114fd49040b6
Test scalar semantics when pending operations are applied. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/6be2c9f129c9
Document scalar semantics during persistence deserialization. r=Dexter

Comment 36

[Raul Gurzau (:RaulG)]

Backed out 3 changesets (bug 1454606) for bustage on /toolkit/components/telemetry/TelemetryScalar.cpp on a CLOSED TREE

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

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

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

Log snippet: 
[task 2018-05-30T10:53:06.331Z] 10:53:06     INFO -  1 warning generated.
[task 2018-05-30T10:53:06.331Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/pk11wrap/pk11wrap_pk11wrap'
[task 2018-05-30T10:53:06.331Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/pk11wrap/pk11wrap_pk11wrap'
[task 2018-05-30T10:53:06.332Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/pk11wrap/pk11wrap_pk11wrap'
[task 2018-05-30T10:53:06.350Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/vr/openvr'
[task 2018-05-30T10:53:06.351Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -std=gnu++14 -o envvartools_public.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -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 -DVR_API_PUBLIC -DPOSIX -DLINUX -DLINUX64 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/gfx/vr/openvr -I/builds/worker/workspace/build/src/obj-firefox/gfx/vr/openvr -I/builds/worker/workspace/build/src/toolkit/components/jsoncpp/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 -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 -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wloop-analysis -Wc++1z-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -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 -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 -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer -Werror -Wno-error=parentheses -Wno-error=unused-variable  -MD -MP -MF .deps/envvartools_public.o.pp   /builds/worker/workspace/build/src/gfx/vr/openvr/src/envvartools_public.cpp
[task 2018-05-30T10:53:06.351Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/vr/openvr'
[task 2018-05-30T10:53:06.351Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/vr/openvr'
[task 2018-05-30T10:53:06.351Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/vr/openvr'
[task 2018-05-30T10:53:06.351Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/ssl/ssl_ssl'
[task 2018-05-30T10:53:06.355Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang -std=gnu99 -o sslauth.o -c -DNDEBUG -DTRIMMED=1 -DNSS_ALLOW_SSLKEYLOGFILE=1 -DNSS_FIPS_DISABLED -DNSS_NO_INIT_SUPPORT -DNSS_X86_OR_X64 -DNSS_X64 -DNSS_USE_64 -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DLINUX2_1 -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -D_REENTRANT -DNSS_DISABLE_LIBPKIX -I/builds/worker/workspace/build/src/security/nss/lib/ssl -I/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/ssl/ssl_ssl -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/private/nss -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -I/builds/worker/workspace/build/src/obj-firefox/dist/include -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 -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wclass-varargs -Wloop-analysis -Werror=non-literal-null-conversion -Wstring-conversion -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer  -MD -MP -MF .deps/sslauth.o.pp   /builds/worker/workspace/build/src/security/nss/lib/ssl/sslauth.c
[task 2018-05-30T10:53:06.355Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/ssl/ssl_ssl'
[task 2018-05-30T10:53:06.355Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/vr/openvr'
[task 2018-05-30T10:53:06.355Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/vr/openvr'
[task 2018-05-30T10:53:06.371Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/vr/openvr'
[task 2018-05-30T10:53:06.371Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/vr/openvr'
[task 2018-05-30T10:53:06.371Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/vr/openvr'
[task 2018-05-30T10:53:06.371Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/vr/openvr'
[task 2018-05-30T10:53:06.401Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/angle/targets/preprocessor'
[task 2018-05-30T10:53:06.401Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -std=gnu++14 -o DirectiveHandlerBase.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -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 -D__NDK_FPABI__= -Dconstexpr14= -DANGLE_SKIP_DXGI_1_2_CHECK -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DEGL_EGLEXT_PROTOTYPES -DGL_GLEXT_PROTOTYPES -DNOMINMAX -DNTDDI_VERSION=0x0A000000 -DUNICODE -D_ATL_NO_OPENGL -D_CRT_RAND_S -D_CRT_SECURE_NO_DEPRECATE -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SECURE_ATL -D_UNICODE -I/builds/worker/workspace/build/src/gfx/angle/targets/preprocessor -I/builds/worker/workspace/build/src/obj-firefox/gfx/angle/targets/preprocessor -I/builds/worker/workspace/build/src/gfx/angle/checkout -I/builds/worker/workspace/build/src/gfx/angle/checkout/include -I/builds/worker/workspace/build/src/gfx/angle/checkout/out/gen -I/builds/worker/workspace/build/src/gfx/angle/checkout/out/gen/angle -I/builds/worker/workspace/build/src/gfx/angle/checkout/src -I/builds/worker/workspace/build/src/gfx/angle/checkout/src/common/third_party/base -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 -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wloop-analysis -Wc++1z-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -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 -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 -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer -msse2  -MD -MP -MF .deps/DirectiveHandlerBase.o.pp   /builds/worker/workspace/build/src/gfx/angle/checkout/src/compiler/preprocessor/DirectiveHandlerBase.cpp
[task 2018-05-30T10:53:06.401Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/angle/targets/preprocessor'
[task 2018-05-30T10:53:06.401Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/angle/targets/preprocessor'
[task 2018-05-30T10:53:06.402Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/angle/targets/preprocessor'
[task 2018-05-30T10:53:06.405Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/gfx/angle/targets/preprocessor'
[task 2018-05-30T10:53:06.405Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/gfx/angle/targets/preprocessor'
[task 2018-05-30T10:53:06.447Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/pk11wrap/pk11wrap_pk11wrap'
[task 2018-05-30T10:53:06.452Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang -std=gnu99 -o pk11pars.o -c -DNDEBUG -DTRIMMED=1 '-DSHLIB_SUFFIX="so"' '-DSHLIB_PREFIX="lib"' '-DSHLIB_VERSION="3"' '-DSOFTOKEN_SHLIB_VERSION="3"' -DNSS_FIPS_DISABLED -DNSS_NO_INIT_SUPPORT -DNSS_X86_OR_X64 -DNSS_X64 -DNSS_USE_64 -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DLINUX2_1 -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -D_REENTRANT -DNSS_DISABLE_LIBPKIX -I/builds/worker/workspace/build/src/security/nss/lib/pk11wrap -I/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/pk11wrap/pk11wrap_pk11wrap -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/private/nss -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -I/builds/worker/workspace/build/src/obj-firefox/dist/include -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 -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wclass-varargs -Wloop-analysis -Werror=non-literal-null-conversion -Wstring-conversion -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer  -MD -MP -MF .deps/pk11pars.o.pp   /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11pars.c
[task 2018-05-30T10:53:06.452Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11pars.c:463:23: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
[task 2018-05-30T10:53:06.452Z] 10:53:06     INFO -          for (i = 0; i < PR_ARRAY_SIZE(policyFlagList); i++) {
[task 2018-05-30T10:53:06.452Z] 10:53:06     INFO -                      ~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2018-05-30T10:53:06.452Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11pars.c:466:36: warning: comparison of integers of different signs: 'const unsigned int' and 'int' [-Wsign-compare]
[task 2018-05-30T10:53:06.452Z] 10:53:06     INFO -              if ((policy->name_size == length) &&
[task 2018-05-30T10:53:06.452Z] 10:53:06     INFO -                   ~~~~~~~~~~~~~~~~~ ^  ~~~~~~
[task 2018-05-30T10:53:06.452Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11pars.c:487:19: warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
[task 2018-05-30T10:53:06.452Z] 10:53:06     INFO -      for (i = 0; i < PR_ARRAY_SIZE(sslOptList); i++) {
[task 2018-05-30T10:53:06.452Z] 10:53:06     INFO -                  ~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2018-05-30T10:53:06.456Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11pars.c:488:31: warning: comparison of integers of different signs: 'int' and 'const unsigned int' [-Wsign-compare]
[task 2018-05-30T10:53:06.456Z] 10:53:06     INFO -          if (policyValueLength == sslOptList[i].name_size &&
[task 2018-05-30T10:53:06.459Z] 10:53:06     INFO -              ~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~
[task 2018-05-30T10:53:06.459Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11pars.c:552:25: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
[task 2018-05-30T10:53:06.459Z] 10:53:06     INFO -              if ((length >= name_size) && (cipher[name_size] == '/')) {
[task 2018-05-30T10:53:06.459Z] 10:53:06     INFO -                   ~~~~~~ ^  ~~~~~~~~~
[task 2018-05-30T10:53:06.459Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11pars.c:555:49: warning: comparison of integers of different signs: 'const unsigned int' and 'int' [-Wsign-compare]
[task 2018-05-30T10:53:06.460Z] 10:53:06     INFO -              if ((newOption || algOpt->name_size == length) &&
[task 2018-05-30T10:53:06.460Z] 10:53:06     INFO -                                ~~~~~~~~~~~~~~~~~ ^  ~~~~~~
[task 2018-05-30T10:53:06.460Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11pars.c:588:25: warning: comparison of integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
[task 2018-05-30T10:53:06.460Z] 10:53:06     INFO -              if ((length > name_size) && cipher[name_size] == '=' &&
[task 2018-05-30T10:53:06.460Z] 10:53:06     INFO -                   ~~~~~~ ^ ~~~~~~~~~
[task 2018-05-30T10:53:06.461Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11pars.c:1416:20: warning: comparison of integers of different signs: 'CK_SLOT_ID' (aka 'unsigned long') and 'int' [-Wsign-compare]
[task 2018-05-30T10:53:06.461Z] 10:53:06     INFO -          if (ids[i] == -1) {
[task 2018-05-30T10:53:06.461Z] 10:53:06     INFO -              ~~~~~~ ^  ~~
[task 2018-05-30T10:53:06.461Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/security/nss/lib/pk11wrap/pk11pars.c:1453:20: warning: comparison of integers of different signs: 'CK_SLOT_ID' (aka 'unsigned long') and 'int' [-Wsign-compare]
[task 2018-05-30T10:53:06.461Z] 10:53:06     INFO -          if (ids[i] == -1) {
[task 2018-05-30T10:53:06.464Z] 10:53:06     INFO -              ~~~~~~ ^  ~~
[task 2018-05-30T10:53:06.465Z] 10:53:06     INFO -  9 warnings generated.
[task 2018-05-30T10:53:06.465Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/pk11wrap/pk11wrap_pk11wrap'
[task 2018-05-30T10:53:06.465Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/pk11wrap/pk11wrap_pk11wrap'
[task 2018-05-30T10:53:06.465Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/pk11wrap/pk11wrap_pk11wrap'
[task 2018-05-30T10:53:06.513Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/ssl/ssl_ssl'
[task 2018-05-30T10:53:06.514Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang -std=gnu99 -o sslbloom.o -c -DNDEBUG -DTRIMMED=1 -DNSS_ALLOW_SSLKEYLOGFILE=1 -DNSS_FIPS_DISABLED -DNSS_NO_INIT_SUPPORT -DNSS_X86_OR_X64 -DNSS_X64 -DNSS_USE_64 -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DLINUX2_1 -DLINUX -Dlinux -DHAVE_STRERROR -DXP_UNIX -D_REENTRANT -DNSS_DISABLE_LIBPKIX -I/builds/worker/workspace/build/src/security/nss/lib/ssl -I/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/ssl/ssl_ssl -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/private/nss -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -I/builds/worker/workspace/build/src/obj-firefox/dist/include -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 -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wclass-varargs -Wloop-analysis -Werror=non-literal-null-conversion -Wstring-conversion -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wformat -Wformat-security -Wno-gnu-zero-variadic-macro-arguments -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe -g -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer  -MD -MP -MF .deps/sslbloom.o.pp   /builds/worker/workspace/build/src/security/nss/lib/ssl/sslbloom.c
[task 2018-05-30T10:53:06.514Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/ssl/ssl_ssl'
[task 2018-05-30T10:53:06.516Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/ssl/ssl_ssl'
[task 2018-05-30T10:53:06.516Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/security/nss/lib/ssl/ssl_ssl'
[task 2018-05-30T10:53:06.536Z] 10:53:06     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/components/telemetry'
[task 2018-05-30T10:53:06.544Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/clang/bin/clang++ -std=gnu++14 -o TelemetryScalar.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -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 -DOS_POSIX=1 -DOS_LINUX=1 '-DMOZ_APP_VERSION="62.0a1"' -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/toolkit/components/telemetry -I/builds/worker/workspace/build/src/obj-firefox/toolkit/components/telemetry -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/xpcom/build -I/builds/worker/workspace/build/src/xpcom/threads -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 -Wsign-compare -Wtype-limits -Wunreachable-code -Wunreachable-code-return -Wwrite-strings -Wno-invalid-offsetof -Wclass-varargs -Wloop-analysis -Wc++1z-compat -Wcomma -Wimplicit-fallthrough -Werror=non-literal-null-conversion -Wstring-conversion -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 -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 -Xclang -load -Xclang /builds/worker/workspace/build/src/obj-firefox/build/clang-plugin/libclang-plugin.so -Xclang -add-plugin -Xclang moz-check -O2 -fno-omit-frame-pointer -Werror -Wno-error=shadow  -MD -MP -MF .deps/TelemetryScalar.o.pp   /builds/worker/workspace/build/src/toolkit/components/telemetry/TelemetryScalar.cpp
[task 2018-05-30T10:53:06.544Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/toolkit/components/telemetry/TelemetryScalar.cpp:1909:55: error: Type 'mozilla::Maybe<ProcessID>' must not be used as parameter
[task 2018-05-30T10:53:06.544Z] 10:53:06     INFO -                              mozilla::Maybe<ProcessID> aProcessType = Nothing())
[task 2018-05-30T10:53:06.545Z] 10:53:06     INFO -                                                        ^
[task 2018-05-30T10:53:06.545Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/toolkit/components/telemetry/TelemetryScalar.cpp:1909:55: note: Please consider passing a const reference instead
[task 2018-05-30T10:53:06.545Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Maybe.h:167:69: note: 'mozilla::Maybe<ProcessID>' is a non-param type because member 'mStorage' has an alignas(_) annotation
[task 2018-05-30T10:53:06.545Z] 10:53:06     INFO -  class MOZ_NON_PARAM MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS Maybe
[task 2018-05-30T10:53:06.545Z] 10:53:06     INFO -                                                                      ^
[task 2018-05-30T10:53:06.546Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/toolkit/components/telemetry/TelemetryScalar.cpp:2005:60: error: Type 'mozilla::Maybe<ProcessID>' must not be used as parameter
[task 2018-05-30T10:53:06.546Z] 10:53:06     INFO -                                   mozilla::Maybe<ProcessID> aProcessType = Nothing())
[task 2018-05-30T10:53:06.546Z] 10:53:06     INFO -                                                             ^
[task 2018-05-30T10:53:06.546Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/toolkit/components/telemetry/TelemetryScalar.cpp:2005:60: note: Please consider passing a const reference instead
[task 2018-05-30T10:53:06.546Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Maybe.h:167:69: note: 'mozilla::Maybe<ProcessID>' is a non-param type because member 'mStorage' has an alignas(_) annotation
[task 2018-05-30T10:53:06.547Z] 10:53:06     INFO -  class MOZ_NON_PARAM MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS Maybe
[task 2018-05-30T10:53:06.547Z] 10:53:06     INFO -                                                                      ^
[task 2018-05-30T10:53:06.547Z] 10:53:06     INFO -  2 errors generated.
[task 2018-05-30T10:53:06.547Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1031: recipe for target 'TelemetryScalar.o' failed
[task 2018-05-30T10:53:06.548Z] 10:53:06     INFO -  make[4]: *** [TelemetryScalar.o] Error 1
[task 2018-05-30T10:53:06.548Z] 10:53:06     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/toolkit/components/telemetry'
[task 2018-05-30T10:53:06.548Z] 10:53:06     INFO -  /builds/worker/workspace/build/src/config/recurse.mk:73: recipe for target 'toolkit/components/telemetry/target' failed
[task 2018-05-30T10:53:06.548Z] 10:53:06     INFO -  make[3]: *** [toolkit/components/telemetry/target] Error 2
[task 2018-05-30T10:53:06.548Z] 10:53:06     INFO -  make[3]: *** Waiting for unfinished jobs....

Comment 37

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979885 [details]
Bug 1454606 - Record scalar operations and apply operations after load.

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

Comment 38

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979886 [details]
Bug 1454606 - Test scalar semantics when pending operations are applied.

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

Comment 39

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8981382 [details]
Bug 1454606 - Document scalar semantics during persistence deserialization.

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

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b126c9d96aaf
Record scalar operations and apply operations after load. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/1df93e12f6ec
Test scalar semantics when pending operations are applied. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/bac711e0ab42
Document scalar semantics during persistence deserialization. r=Dexter

Comment 41

[Raul Gurzau (:RaulG)]

Backed out 3 changesets (bug 1454606) for assert failure on /build/src/obj-firefox/dist/include/js/Value.h

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

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

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

Log snippet: 
07:47:54     INFO -  TEST-START | TelemetryGeckoViewFixture.ClearPersistenceFiles
07:47:54     INFO -  **** TELEMETRY: InitPersistence
07:47:54     INFO -  **** TELEMETRY: ClearPersistenceData
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadLoadData
07:47:54     INFO -  **** TELEMETRY: DeInitPersistence
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: MainThreadParsePersistedProbes
07:47:54     INFO -  **** TELEMETRY: MainThreadParsePersistedProbes - Failed to parse 'histograms', NS_OK.
07:47:54     INFO -  **** TELEMETRY: MainThreadParsePersistedProbes - Failed to parse 'keyedHistograms', NS_OK.
07:47:54     INFO -  **** TELEMETRY: MainThreadArmPersistenceTimerTEST-PASS | TelemetryGeckoViewFixture.ClearPersistenceFiles | test completed (time: 3ms)
07:47:54     INFO -  TEST-START | TelemetryGeckoViewFixture.CheckDataLoadedTopic
07:47:54     INFO -  **** TELEMETRY: InitPersistence
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadLoadData
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadLoadData - Failed to load cache file at /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: MainThreadArmPersistenceTimer
07:47:54     INFO -  **** TELEMETRY: DeInitPersistence
07:47:54     INFO -  **** TELEMETRY: InitPersistence
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadLoadData
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: MainThreadParsePersistedProbes
07:47:54     INFO -  **** TELEMETRY: MainThreadParsePersistedProbes - Failed to parse 'histograms', NS_OK.
07:47:54     INFO -  **** TELEMETRY: MainThreadParsePersistedProbes - Failed to parse 'keyedHistograms', NS_OK.
07:47:54     INFO -  **** TELEMETRY: MainThreadArmPersistenceTimer
07:47:54     INFO -  **** TELEMETRY: DeInitPersistenceTEST-PASS | TelemetryGeckoViewFixture.CheckDataLoadedTopic | test completed (time: 9ms)
07:47:54     INFO -  TEST-START | TelemetryGeckoViewFixture.PersistScalars
07:47:54     INFO -  **** TELEMETRY: InitPersistence
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadLoadData
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadLoadData - Failed to load cache file at /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: MainThreadArmPersistenceTimer
07:47:54     INFO -  **** TELEMETRY: DeInitPersistence
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadPersist
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: MainThreadArmPersistenceTimer
07:47:54     INFO -  **** TELEMETRY: InitPersistence
07:47:54     INFO -  **** TELEMETRY: DeInitPersistence
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadLoadData
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: MainThreadParsePersistedProbes
07:47:54     INFO -  **** TELEMETRY: MainThreadArmPersistenceTimerTEST-PASS | TelemetryGeckoViewFixture.PersistScalars | test completed (time: 128ms)
07:47:54     INFO -  TEST-START | TelemetryGeckoViewFixture.PersistHistograms
07:47:54     INFO -  **** TELEMETRY: InitPersistence
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadLoadData
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadLoadData - Failed to load cache file at /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: MainThreadArmPersistenceTimer
07:47:54     INFO -  **** TELEMETRY: DeInitPersistence
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadPersist
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: MainThreadArmPersistenceTimer
07:47:54     INFO -  **** TELEMETRY: InitPersistence
07:47:54     INFO -  **** TELEMETRY: DeInitPersistence
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadLoadData
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: MainThreadParsePersistedProbes
07:47:54     INFO -  **** TELEMETRY: MainThreadArmPersistenceTimerTEST-PASS | TelemetryGeckoViewFixture.PersistHistograms | test completed (time: 143ms)
07:47:54     INFO -  TEST-START | TelemetryGeckoViewFixture.TimerHitCountProbe
07:47:54     INFO -  **** TELEMETRY: InitPersistence
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadLoadData
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadLoadData - Failed to load cache file at /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: MainThreadArmPersistenceTimer
07:47:54     INFO -  **** TELEMETRY: DeInitPersistence
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadPersist
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: PersistenceThreadPersist
07:47:54     INFO -  **** TELEMETRY: GetPersistenceFile -  /var/folders/g2/clpwb6c101gfxkq73b50s8p400000w/T/gv_measurements.json
07:47:54     INFO -  **** TELEMETRY: MainThreadArmPersistenceTimer
07:47:54     INFO -  **** TELEMETRY: MainThreadArmPersistenceTimerTEST-PASS | TelemetryGeckoViewFixture.TimerHitCountProbe | test completed (time: 228ms)
07:47:54     INFO -  TEST-START | TelemetryGeckoViewFixture.EmptyPendingOperations
07:47:54     INFO -  Assertion failure: isObject(), at /builds/worker/workspace/build/src/obj-firefox/dist/include/js/Value.h:792

Comment 42

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8979886 [details]
Bug 1454606 - Test scalar semantics when pending operations are applied.

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

Comment 43

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8981382 [details]
Bug 1454606 - Document scalar semantics during persistence deserialization.

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

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/02dbf7b85baa
Record scalar operations and apply operations after load. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/f501668b9cf9
Test scalar semantics when pending operations are applied. r=Dexter
https://hg.mozilla.org/integration/autoland/rev/c81af374dfc7
Document scalar semantics during persistence deserialization. r=Dexter

Comment 45

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/02dbf7b85baa
https://hg.mozilla.org/mozilla-central/rev/f501668b9cf9
https://hg.mozilla.org/mozilla-central/rev/c81af374dfc7