Bug 202487 - Web Inspector: remove `InspectorInstrumentationCookie`
Summary: Web Inspector: remove `InspectorInstrumentationCookie`
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-02 12:34 PDT by Devin Rousso
Modified: 2019-10-08 17:10 PDT (History)
12 users (show)

See Also:


Attachments
Patch (79.26 KB, patch)
2019-10-02 12:46 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (79.29 KB, patch)
2019-10-03 09:23 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-10-02 12:34:08 PDT
All it does is create a shallow-copy of the `InstrumentingAgents` (it copies a pointer to the `InstrumentingAgents` object, so any of the pointers to the `InspectorAgent` that are contained therein are shared between each instance) and save the tracking `InspectorTimelineAgent`'s id (monotonically increasing integer).  All uses of `InspectorInstrumentationCookie` are local values, so there's no way that the `InspectorTimelineAgent` (or the id) could change in between an `InspectorInstrumentation::will*` and an `InspectorInstrumentation::did*`, as both the tracking state and id are controlled by the frontend and cannot "interrupt".
Comment 1 Devin Rousso 2019-10-02 12:46:53 PDT
Created attachment 380048 [details]
Patch
Comment 2 Matt Baker 2019-10-02 16:36:03 PDT
Comment on attachment 380048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=380048&action=review

r=me, nice refactor.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:401
> +        return;

Breaking out this early return is nice. :)

> Source/WebCore/inspector/InspectorInstrumentation.h:835
> +

There is an empty line after FAST_RETURN_IF_NO_FRONTENDS here, and below (843), but not elsewhere. We should be consistent.
Comment 3 Devin Rousso 2019-10-03 09:23:27 PDT
Created attachment 380122 [details]
Patch
Comment 4 WebKit Commit Bot 2019-10-03 12:38:59 PDT
Comment on attachment 380122 [details]
Patch

Clearing flags on attachment: 380122

Committed r250672: <https://trac.webkit.org/changeset/250672>
Comment 5 WebKit Commit Bot 2019-10-03 12:39:01 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2019-10-03 12:39:17 PDT
<rdar://problem/55957063>
Comment 7 Truitt Savell 2019-10-08 10:41:54 PDT
It looks like the changes in https://trac.webkit.org/changeset/250672/webkit

broke inspector/timeline/timeline-recording.html

It is crashing on Mac debug and failing on Release.

I was able to reproduce the crashes by simply running the test on 250672 which crashes, and did not crash on 250668 which was the revision before it. cherry picked were in-between.
Comment 8 Devin Rousso 2019-10-08 17:10:46 PDT
(In reply to Truitt Savell from comment #7)
> It looks like the changes in https://trac.webkit.org/changeset/250672/webkit
> 
> broke inspector/timeline/timeline-recording.html
> 
> It is crashing on Mac debug and failing on Release.
> 
> I was able to reproduce the crashes by simply running the test on 250672 which crashes, and did not crash on 250668 which was the revision before it. cherry picked were in-between.
I'm addressing this in <https://webkit.org/b/202713>.