Add a common mechanism for identifying obsolete client operations

RESOLVED FIXED in Firefox 68

Status

enhancement
P3
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Blocks 1 bug)

unspecified
Firefox 68
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

2 months ago
Posted patch WIP (obsolete) — Splinter Review

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.

Priority: -- → P3
Status: NEW → ASSIGNED
Assignee

Comment 1

2 months ago
Posted patch WIP (obsolete) — Splinter Review

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.

Attachment #9051024 - Attachment is obsolete: true
Assignee

Comment 2

2 months ago

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

Assignee

Comment 3

2 months ago
Posted patch WIPSplinter Review

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.

Attachment #9051084 - Attachment is obsolete: true

Comment 5

2 months ago
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d363668ebdaa
Add a Context object for rejecting out of date client operations, r=loganfsmyth.

Comment 7

2 months ago
Backout by dvarga@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/eb21297f3159
Backed out 2 changesets for causing merge conflicts. a=backout

Comment 9

2 months ago
Pushed by bhackett@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27111bcac93a
Add a Context object for rejecting out of date client operations, r=loganfsmyth.

Comment 10

2 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
Assignee

Updated

a month ago
Flags: needinfo?(bhackett1024)
You need to log in before you can comment on or make changes to this bug.