Bug 216385 - Web Inspector: Change `InspectorAnimationAgent->startTracking` to not error on repeated calls
Summary: Web Inspector: Change `InspectorAnimationAgent->startTracking` to not error o...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Patrick Angle
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-10 16:03 PDT by BJ Burg
Modified: 2020-09-22 21:47 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.63 KB, patch)
2020-09-22 08:21 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2020-09-10 16:03:54 PDT
STEPS TO REPRODUCE:
- navigate to brrian.org
- open inspector to Timelines tab, with Media & Animations timeline enabled
- navigate to nfl.com
- Uncaught exception "Animation domain already tracking (at Connection.js:​162:​29)"


seems like animation instrumentation is auto-starting, or has bad initial state upon cross-domain navigation. I wonder if it happens on a same-domain or same-document navigation?
Comment 1 Radar WebKit Bug Importer 2020-09-10 16:15:25 PDT
<rdar://problem/68670991>
Comment 2 Patrick Angle 2020-09-21 09:48:47 PDT
I'm unable to reproduce this issue on same-domain navigation. It seems to be limited to cross-domain navigation.
Comment 3 Joseph Pecoraro 2020-09-21 13:30:48 PDT
This seems likely to be harmless.

Whenever the Timeline Tab is in the foreground the frontend notifies the backend of the open set of timelines so that the backend can automatically start an appropriate timeline recording on page navigations (`Timeline.setAutoCaptureEnabled` and `Timeline.setInstruments`).

What might happen on cross domain navigation is that both the backend and frontend try to start a recording? It seems the Animation domain errors if startTracking is called multiple times, but most other timelines don't (Memory, Heap, CPUProfiler, ScriptProfiler, etc.).
Comment 4 Devin Rousso 2020-09-21 13:39:57 PDT
(In reply to Joseph Pecoraro from comment #3)
> This seems likely to be harmless.
To expand on this further, whenever you see the uncaught exception UI in Web Inspector in an engineering build, it can mean one of two things:
 1) an exception in JavaScript was thrown
 2) some protocol command sent an error payload/message back to the frontend
In the case of (1), that's definitely a bug and should be fixed.
In the case of (2), most of the commands we invoke do not have error handling cases because we don't ever expect them to error.  Most of the time the error is harmless and will not affect the user in any negative way (usually if it does affect the user then we'd see an actual JS error like (1) right after the (2) error).  A simple way to tell (most of the time) is to just hit the "close" link to dismiss the uncaught exception UI and try to continue using Web Inspector as normal (if nothing is wrong, then the error is likely harmless)

> What might happen on cross domain navigation is that both the backend and frontend try to start a recording? It seems the Animation domain errors if startTracking is called multiple times, but most other timelines don't (Memory, Heap, CPUProfiler, ScriptProfiler, etc.).
Yeah I think we should just make `Animation.startTracking` not error.
Comment 5 Patrick Angle 2020-09-21 14:21:53 PDT
(In reply to Devin Rousso from comment #4)
> (In reply to Joseph Pecoraro from comment #3)
> > This seems likely to be harmless.
> To expand on this further, whenever you see the uncaught exception UI in Web
> Inspector in an engineering build, it can mean one of two things:
>  1) an exception in JavaScript was thrown
>  2) some protocol command sent an error payload/message back to the frontend
> In the case of (1), that's definitely a bug and should be fixed.
> In the case of (2), most of the commands we invoke do not have error
> handling cases because we don't ever expect them to error.  Most of the time
> the error is harmless and will not affect the user in any negative way
> (usually if it does affect the user then we'd see an actual JS error like
> (1) right after the (2) error).  A simple way to tell (most of the time) is
> to just hit the "close" link to dismiss the uncaught exception UI and try to
> continue using Web Inspector as normal (if nothing is wrong, then the error
> is likely harmless)

I understand that most of these are harmless, but the noise makes it difficult to identify what is actually a bug and what is "expected" behavior.

> > What might happen on cross domain navigation is that both the backend and frontend try to start a recording? It seems the Animation domain errors if startTracking is called multiple times, but most other timelines don't (Memory, Heap, CPUProfiler, ScriptProfiler, etc.).
> Yeah I think we should just make `Animation.startTracking` not error.

This is certainly a relatively quick fix then.
Comment 6 Patrick Angle 2020-09-22 08:21:23 PDT
Created attachment 409360 [details]
Patch
Comment 7 Devin Rousso 2020-09-22 11:17:05 PDT
Comment on attachment 409360 [details]
Patch

r=me :)
Comment 8 EWS 2020-09-22 21:47:46 PDT
Committed r267464: <https://trac.webkit.org/changeset/267464>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409360 [details].