Bug 216385 – Web Inspector: Change `InspectorAnimationAgent->startTracking` to not error on repeated calls

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

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?

[Radar WebKit Bug Importer]

<rdar://problem/68670991>

[Patrick Angle]

I'm unable to reproduce this issue on same-domain navigation. It seems to be limited to cross-domain navigation.

[Joseph Pecoraro]

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.).

[Devin Rousso]

(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.

[Patrick Angle]

(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.

[Patrick Angle]

Created attachment 409360 [details]
Patch

[Devin Rousso]

Comment on attachment 409360 [details]
Patch

r=me :)

[EWS]

Committed r267464: <https://trac.webkit.org/changeset/267464>

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