Include more context when reporting recording mismatches

RESOLVED FIXED in Firefox 65

Status

()

enhancement
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

unspecified
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(2 attachments)

Posted patch patchSplinter Review
When the events in a thread's stream don't match, we report the names of the recorded and replayed events.  Any additional data associated with the event is not printed, which is not ideal.  In particular, if we recorded a RecordReplayAssert assertion and replayed something else, the error message does not include the string that was asserted in the recording.

The attached patch fixes this so that when the recorded event is one of these assertions, the asserted string is also included in the error message.  The patch also changes the redirection for getenv so that an assertion with the environment string is inserted, which should make it much easier to diagnose recording mismatches involving getenv (one of which I've seen while testing locally).
Attachment #9018832 - Flags: review?(continuation)
Comment on attachment 9018832 [details] [diff] [review]
patch

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

This patch should get data collection review.

::: toolkit/recordreplay/File.cpp
@@ +164,5 @@
> +        // the writes in RecordReplayAssert.
> +        if (mNameIndex == MainThreadId) {
> +          (void) ReadScalar(); // For the ExecutionProgressCounter write below.
> +        }
> +        extra = ReadInputString();

This gets put into the RecordReplayError field, which requires PII access, right? This looks like it can have URL information in it. (RecordReplayAssert("BeginContentParse %s", aURL))

::: toolkit/recordreplay/ProcessRedirectDarwin.cpp
@@ +1350,5 @@
> +{
> +  // Include the environment variable being checked in an assertion, to make it
> +  // easier to debug recording mismatches involving getenv.
> +  auto env = aArguments->Arg<0, const char*>();
> +  RecordReplayAssert("getenv %s", env);

Is this message only logged locally, or does it get inserted into the crash report? Am I reading this right in that we're recording the name of the environment variable and not the value? That seems okay because presumably we are only looking at some fixed set of env vars in Firefox.
Attachment #9018832 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #1)
> Comment on attachment 9018832 [details] [diff] [review]
> patch
> 
> Review of attachment 9018832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch should get data collection review.
> 
> ::: toolkit/recordreplay/File.cpp
> @@ +164,5 @@
> > +        // the writes in RecordReplayAssert.
> > +        if (mNameIndex == MainThreadId) {
> > +          (void) ReadScalar(); // For the ExecutionProgressCounter write below.
> > +        }
> > +        extra = ReadInputString();
> 
> This gets put into the RecordReplayError field, which requires PII access,
> right? This looks like it can have URL information in it.
> (RecordReplayAssert("BeginContentParse %s", aURL))

Yes, the RecordReplayErrror field requires PII access.  I can remove the URL information in those assertions if you want, there aren't many uses of RecordReplayAssert in the tree and the fix in this bug is mainly useful when inserting extra assertions and testing locally.

> ::: toolkit/recordreplay/ProcessRedirectDarwin.cpp
> @@ +1350,5 @@
> > +{
> > +  // Include the environment variable being checked in an assertion, to make it
> > +  // easier to debug recording mismatches involving getenv.
> > +  auto env = aArguments->Arg<0, const char*>();
> > +  RecordReplayAssert("getenv %s", env);
> 
> Is this message only logged locally, or does it get inserted into the crash
> report? Am I reading this right in that we're recording the name of the
> environment variable and not the value? That seems okay because presumably
> we are only looking at some fixed set of env vars in Firefox.

It will be included in the crash report.  We are asserting on the name of the environment variable, and not its value.
Comment on attachment 9018832 [details] [diff] [review]
patch

Asking for data collection review per comment 1.  This patch adds some more data available in crash reports to users with PII access:

- Strings included in record/replay assertions are included more often (they are already included sometimes).

- The names (but not the contents) of environment variables being accessed by getenv calls can be included.
Attachment #9018832 - Flags: review?(francois)
Comment on attachment 9018832 [details] [diff] [review]
patch

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

Brian, could you please email fx-datastewards@mozilla.com with this request?

I don't know what the process is for reviewing crash data collection. Hopefully someone else on the team will know.
Attachment #9018832 - Flags: review?(francois)
Attachment #9025893 - Flags: review?(chutten)
Comment on attachment 9025893 [details]
data collection request

DATA COLLECTION REVIEW RESPONSE:

    Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate? 

This bug will serve as documentation for this collection. The broader crash reporting mechanism has information included in its UI to aid comprehension.

    Is there a control mechanism that allows the user to turn the data collection on and off? 

Yes. Disabling Web Replay (presumably via about:config) or disabling crash reporting.

    If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes, :bhackett.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 3.

    Is the data collection request for default-on or default-off?

Default off. Users must consent to sending crash reports and may opt in to sending them.

    Does the instrumentation include the addition of any new identifiers (whether anonymous or otherwise; e.g., username, random IDs, etc. See the appendix for more details)?

No.

    Is the data collection covered by the existing Firefox privacy notice? 

Yes.

    Does there need to be a check-in in the future to determine whether to renew the data? 

N/A, the collection is permanent.

---
Result: datareview+
Attachment #9025893 - Flags: review?(chutten) → review+
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b12b2142017a
Include more context when reporting recording mismatches, r=mccr8.
https://hg.mozilla.org/mozilla-central/rev/b12b2142017a
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.