1535362 - Add a common mechanism for identifying obsolete client operations

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P3)

Description

[Brian Hackett [Laid off!]]

Attachment: WIP

Many debugger client operations are done asynchronously: they require communicating with the server or other resources, and should not lock up the UI. If the user operates interacts with the UI and navigates to a new page, resumes/pauses a thread, or changes the selected thread, the client operation might no longer be relevant. If it continues to make changes to the redux state, it can corrupt the state with old/invalid information. This has caused several problems lately that were very difficult to track down, and it would be nice to have a general way of dealing with this situation.

The attached patch is a WIP in this direction. The reducer pause state contains a Context object, which encapsulates the core debugger state: how many navigations have occurred, which thread is currently selected, and how many times that thread has paused or resumed. This gives enough information to be able to classify operations as obsolete in several different ways: if a navigation has occurred, if the selected thread has changed, or if the thread has changed its pause state. The idea here is that when an operation is initiated (from a component usually, or from an unsolicited server message) we get the current context, and check that context later on against the current one as the operation proceeds to see if it is irrelevant. This patch adds some middleware to automatically perform these checks on dispatched actions which have a 'cx' property, and ignore them if so.

The interface used here needs some more work, particularly around how we specify the conditions under which an operation is obsolete. The patch is also pretty minimal so far in actually using contexts, so they might evolve in other ways as the uses get filled in. I'm posting this now because this is pretty exploratory and it would be good to get feedback as things proceed.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: WIP

Updated WIP. This uses Context in some key areas like selecting sources, but the main thing it changes is the interface. Instead of having a single Context type that can be interpreted in multiple ways, there is a NavigateContext type which is invalidated when a navigation occurs, and a ThreadContext type which is invalidated when a navigation occurs or when the selected thread changes, pauses, or resumes. Methods can require a ThreadContext when desired (e.g. mapFrames), which will be enforced by Flow.

Comment 2

[Brian Hackett [Laid off!]]

(In reply to Brian Hackett (:bhackett) from comment #1)

Created attachment 9051084 [details] [diff] [review]
WIP

Updated WIP. This uses Context in some key areas like selecting sources, but the main thing it changes is the interface. Instead of having a single Context type that can be interpreted in multiple ways, there is a NavigateContext type which is invalidated when a navigation occurs, and a ThreadContext type which is invalidated when a navigation occurs or when the selected thread changes, pauses, or resumes. Methods can require a ThreadContext when desired (e.g. mapFrames), which will be enforced by Flow.

I forgot to note here that this change does have an effect on the possible debugger operations: redux changes can only change state for the currently selected thread. For example, a mapFrames() call can only be made on the current thread, and if the current thread changes while the mapFrames() is ongoing then its context will be invalidated and it won't be able to update redux or have other side effects.

I think this is a nice simplification. A little more logic will be necessary when switching threads to initiate asynchronous operations to fill in their state (e.g. mapping frames and scopes) but this seems fine.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: WIP

More extensive WIP that covers most actions and redux state. I'm going to stop working on this until we have consensus about the right direction forward.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: Bug 1535362 - Add a Context object for rejecting out of date client operations, r=jlast,loganfsmyth.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d363668ebdaa
Add a Context object for rejecting out of date client operations, r=loganfsmyth.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/52636fd2ce17
Fix web replay test breakage.

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/eb21297f3159
Backed out 2 changesets for causing merge conflicts. a=backout

Comment 8

[Daniel Varga [:dvarga]]

Pushes with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=d363668ebdaa772feed5c3392fed60432789458f
and: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=52636fd2ce17ba27346db56337dda4fcce3f409d

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27111bcac93a
Add a Context object for rejecting out of date client operations, r=loganfsmyth.

Comment 10

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/27111bcac93a