1505935 - The timeline should only show userland locations

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Jason Laster [:jlast]]

first, if you modify RecordReplayInterface_IsInternalScript in toolkit/recordreplay/ipc/JSControl.cpp, it will affect which scripts we update the progress counter for when they run
right now we only filter out the replay.js file itself (which behaves differently depending on what the user is doing, and we don't want it to affect the progress counter)
but adding other scripts there should be fine, as long as we don't ever set breakpoints in that code (not updating the progress counter means we won't know where we are when we stop within these scripts)
second, replay.js has a simple filter for which scripts it will show to the devtools server and which are considered relevant to the user
this is the considerScript() function
it just filters out scripts with no url or which are chrome:// or resource:// scripts
if this filter and the one in RecordReplayInterface_IsInternalScript matched, I think we would end up with more useful progress counter values

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

This patch makes several changes:

- The logic for whether to update the progress counter when a script runs has been consolidated with the logic for whether to expose a script to the debugger, so that scripts which satisfy one will always satisfy the other.

- A new pref, devtools.recordreplay.includeSystemScripts, can be used to expose resource:// and chrome:// scripts to the debugger.

- The logic for performing content parses has been consolidated, so that NoteContentParse calls are not needed outside the JS engine, except for files that include more than the script text itself (i.e. HTML files).  This is needed to get the sources for resource:// scripts, and seems preferable to sprinkling some more NoteContentParse calls around.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Rename IsInternalScript API.

Rename the IsInternalScript public API to ShouldUpdateProgressCounter, to better reflect its purpose.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Expose scripts to the debugger which have their progress counters updated.

Use ShouldUpdateProgressCounter() when determining whether a script should be exposed to the debugger.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Add pref allowing system scripts to be exposed to the debugger.

Add a pref that will cause all scripts to be exposed to the debugger, except the internal replay.js script.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Inform the record/replay system about all script compilations.

Tell the record/replay system about all content parses that go through BytecodeCompiler, similar to what we do when de-XDR'ing scripts.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: Part 5 - Remove unnecessary NoteContentParse call.

This call to NoteContentParse is unnecessary after part 4.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: Part 6 - Fetch the longest piece of content for a given URL.

After part 4 there can be multiple different content parses for the same URL: an HTML file with inline scripts will have each of those inline scripts separately parsed.  There will also be a content parse for the complete HTML file itself, which is what the debugger wants when it asks for the contents of the URL.  To find that complete file, this patch picks the longest block of text that matches a given URL.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 9024240 [details] [diff] [review]
Part 4 - Inform the record/replay system about all script compilations.

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

Sure. Won't rr be notified twice, though, if this is still done in ScriptLoader::EvaluateScript and also here?

Comment 9

[Jason Orendorff [:jorendorff]]

Never mind, I see that's being removed in the next patch.

Comment 10

[Logan Smyth [:loganfsmyth]]

Comment on attachment 9024237 [details] [diff] [review]
Part 1 - Rename IsInternalScript API.

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

::: mfbt/RecordReplay.cpp
@@ +37,5 @@
>    Macro(InternalThingIndex, size_t, (void* aThing), (aThing))   \
>    Macro(InternalVirtualThingName, const char*, (void* aThing), (aThing)) \
>    Macro(ExecutionProgressCounter, ProgressCounter*, (), ())     \
>    Macro(NewTimeWarpTarget, ProgressCounter, (), ())             \
> +  Macro(ShouldUpdateProgressCounter, bool, (const char* aURL), (aURL)) \

Actually, isn't this patch out of order, since this doesn't exist until Part 2? Not sure if these land in order or what.

Comment 11

[Logan Smyth [:loganfsmyth]]

Comment on attachment 9024243 [details] [diff] [review]
Part 6 - Fetch the longest piece of content for a given URL.

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

Not an issue with this patch specifically, but is there a danger here if this finding the wrong file in some case? It's always possible that the server could return two different scripts for a given URL for one reason or another.

Comment 12

[Brian Hackett [Laid off!]]

(In reply to Logan Smyth [:loganfsmyth] from comment #10)
> Comment on attachment 9024237 [details] [diff] [review]
> Part 1 - Rename IsInternalScript API.
> 
> Review of attachment 9024237 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/RecordReplay.cpp
> @@ +37,5 @@
> >    Macro(InternalThingIndex, size_t, (void* aThing), (aThing))   \
> >    Macro(InternalVirtualThingName, const char*, (void* aThing), (aThing)) \
> >    Macro(ExecutionProgressCounter, ProgressCounter*, (), ())     \
> >    Macro(NewTimeWarpTarget, ProgressCounter, (), ())             \
> > +  Macro(ShouldUpdateProgressCounter, bool, (const char* aURL), (aURL)) \
> 
> Actually, isn't this patch out of order, since this doesn't exist until Part
> 2? Not sure if these land in order or what.

The patches will all land at the same time, I've just split them up into logically related parts (though, yeah, there are still some additional interdependencies).

Comment 13

[Brian Hackett [Laid off!]]

(In reply to Logan Smyth [:loganfsmyth] from comment #11)
> Comment on attachment 9024243 [details] [diff] [review]
> Part 6 - Fetch the longest piece of content for a given URL.
> 
> Review of attachment 9024243 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not an issue with this patch specifically, but is there a danger here if
> this finding the wrong file in some case? It's always possible that the
> server could return two different scripts for a given URL for one reason or
> another.

Hmm, yeah, we are vulnerable to this, and it looks like the situation is worse when replaying than in the normal case.  It would be good to rework things at some point to avoid this problem when possible.

Comment 14

[Logan Smyth [:loganfsmyth]]

Comment on attachment 9024238 [details] [diff] [review]
Part 2 - Expose scripts to the debugger which have their progress counters updated.

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

::: toolkit/recordreplay/ipc/JSControl.cpp
@@ +813,5 @@
> +  if (args.get(0).isNull()) {
> +    args.rval().setBoolean(ShouldUpdateProgressCounter(nullptr));
> +  } else {
> +    if (!args.get(0).isString()) {
> +      JS_ReportErrorASCII(aCx, "Expected string");

Seems more accurate to say "string or null as first argument"?

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd1a91375422
Part 1 - Rename IsInternalScript API, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ebebcc2d183
Part 2 - Expose scripts to the debugger which have their progress counters updated, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f5ccba021a
Part 3 - Add pref allowing system scripts to be exposed to the debugger, r=lsmyth.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7d45d657404
Part 6 - Fetch the longest piece of content for a given URL, r=lsmyth.

Comment 16

[Brian Hackett [Laid off!]]

I landed this without parts 4 and 5 for now, as JS parsing APIs have changed substantially in the last week and part 4 needs to be rewritten.

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fd1a91375422
https://hg.mozilla.org/mozilla-central/rev/4ebebcc2d183
https://hg.mozilla.org/mozilla-central/rev/d0f5ccba021a
https://hg.mozilla.org/mozilla-central/rev/c7d45d657404

Comment 18

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Inform the record/replay system about all script compilations.

Update part 4 for the recent changes to script compilation interfaces.  This also renames NoteContentParse8/16 to just an overloaded NoteContentParse, to be more template friendly.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b12903dd378
Part 4 - Inform the record/replay system about all script compilations, r=jorendorff.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0c382d99e76
Part 5 - Remove unnecessary NoteContentParse call, r=froydnj.

Comment 20

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/2b12903dd378
https://hg.mozilla.org/mozilla-central/rev/d0c382d99e76