The timeline should only show userland locations

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: jlast, Assigned: bhackett)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: leave-open)

Attachments

(7 attachments, 1 obsolete attachment)

Reporter

Description

6 months ago
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
Assignee

Comment 1

6 months ago
Posted patch patchSplinter Review
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.
Assignee: nobody → bhackett1024
Assignee

Comment 2

6 months ago
Rename the IsInternalScript public API to ShouldUpdateProgressCounter, to better reflect its purpose.
Attachment #9024237 - Flags: review?(lsmyth)
Assignee

Comment 3

6 months ago
Use ShouldUpdateProgressCounter() when determining whether a script should be exposed to the debugger.
Attachment #9024238 - Flags: review?(lsmyth)
Assignee

Comment 4

6 months ago
Add a pref that will cause all scripts to be exposed to the debugger, except the internal replay.js script.
Attachment #9024239 - Flags: review?(lsmyth)
Assignee

Comment 5

6 months ago
Tell the record/replay system about all content parses that go through BytecodeCompiler, similar to what we do when de-XDR'ing scripts.
Attachment #9024240 - Flags: review?(jorendorff)
Assignee

Comment 6

6 months ago
This call to NoteContentParse is unnecessary after part 4.
Attachment #9024242 - Flags: review?(nfroyd)
Assignee

Comment 7

6 months ago
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.
Attachment #9024243 - Flags: review?(lsmyth)
Attachment #9024242 - Flags: review?(nfroyd) → review+
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?
Attachment #9024240 - Flags: review?(jorendorff) → review+
Never mind, I see that's being removed in the next patch.
Attachment #9024237 - Flags: review?(lsmyth) → review+
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.
Attachment #9024239 - Flags: review?(lsmyth) → review+
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.
Attachment #9024243 - Flags: review?(lsmyth) → review+
Assignee

Comment 12

6 months ago
(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).
Assignee

Comment 13

6 months ago
(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 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"?
Attachment #9024238 - Flags: review?(lsmyth) → review+

Comment 15

6 months ago
Pushed by bhackett@mozilla.com:
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.
Assignee

Comment 16

6 months ago
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.
Whiteboard: leave-open
Assignee

Comment 18

6 months ago
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.
Attachment #9024240 - Attachment is obsolete: true
Attachment #9025896 - Flags: review?(jorendorff)
Attachment #9025896 - Flags: review?(jorendorff) → review+

Comment 19

6 months ago
Pushed by bhackett@mozilla.com:
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.
Assignee

Updated

6 months ago
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.