Bug 1534847 - Part 2: Invalidate the loadSourceText cache on navigation. r=jlast
☠☠ backed out by fa6af8c14ee4 ☠ ☠
authorLogan Smyth <loganfsmyth@gmail.com>
Thu, 14 Mar 2019 22:04:28 +0000
changeset 521977 27a3400989c9bce15e4c9b1422637067e34e7c6c
parent 521976 7cf6462189e8d4834052afa8fb9201744c5df56a
child 521978 ee3da141383532c5f82be55ea74fc5e0b4622ec8
push id10870
push usernbeleuzu@mozilla.com
push dateFri, 15 Mar 2019 20:00:07 +0000
treeherdermozilla-beta@c594aee5b7a4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjlast
bugs1534847
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1534847 - Part 2: Invalidate the loadSourceText cache on navigation. r=jlast If users navigate while source text is loading, we need to ignore existing cached promises because they may resolve and then not actually set the resulting source, because the source was deleted from the source list. We want to explicitly use a new cache entry if we have navigated. Differential Revision: https://phabricator.services.mozilla.com/D23452
devtools/client/debugger/new/src/actions/sources/loadSourceText.js
devtools/client/debugger/new/src/actions/types/SourceAction.js
devtools/client/debugger/new/src/reducers/sources.js
--- a/devtools/client/debugger/new/src/actions/sources/loadSourceText.js
+++ b/devtools/client/debugger/new/src/actions/sources/loadSourceText.js
@@ -1,16 +1,16 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
 
 // @flow
 
 import { PROMISE } from "../utils/middleware/promise";
-import { getSource } from "../../selectors";
+import { getSource, getSourcesEpoch } from "../../selectors";
 import { setBreakpointPositions } from "../breakpoints";
 
 import * as parser from "../../workers/parser";
 import { isLoaded, isOriginal } from "../../utils/source";
 import { Telemetry } from "devtools-modules";
 
 import type { ThunkArgs } from "../types";
 
@@ -39,25 +39,27 @@ async function loadSource(state, source:
     id: source.id,
     text: response.source,
     contentType: response.contentType || "text/javascript"
   };
 }
 
 async function loadSourceTextPromise(
   source: Source,
+  epoch: number,
   { dispatch, getState, client, sourceMaps }: ThunkArgs
 ): Promise<?Source> {
   if (isLoaded(source)) {
     return source;
   }
 
   await dispatch({
     type: "LOAD_SOURCE_TEXT",
     sourceId: source.id,
+    epoch,
     [PROMISE]: loadSource(getState(), source, { sourceMaps, client })
   });
 
   const newSource = getSource(getState(), source.id);
   if (!newSource) {
     return;
   }
 
@@ -77,22 +79,24 @@ export function loadSourceText(inputSour
   return async (thunkArgs: ThunkArgs) => {
     if (!inputSource) {
       return;
     }
     // This ensures that the falsy check above is preserved into the IIFE
     // below in a way that Flow is happy with.
     const source = inputSource;
 
-    const id = source.id;
+    const epoch = getSourcesEpoch(thunkArgs.getState());
+
+    const id = `${epoch}:${source.id}`;
     let promise = requests.get(id);
     if (!promise) {
       promise = (async () => {
         try {
-          return await loadSourceTextPromise(source, thunkArgs);
+          return await loadSourceTextPromise(source, epoch, thunkArgs);
         } catch (e) {
           // TODO: This swallows errors for now. Ideally we would get rid of
           // this once we have a better handle on our async state management.
         } finally {
           requests.delete(id);
         }
       })();
       requests.set(id, promise);
--- a/devtools/client/debugger/new/src/actions/types/SourceAction.js
+++ b/devtools/client/debugger/new/src/actions/types/SourceAction.js
@@ -5,17 +5,18 @@
 // @flow
 
 import type { Source, SourceLocation } from "../../types";
 import type { PromiseAction } from "../utils/middleware/promise";
 
 export type LoadSourceAction = PromiseAction<
   {|
     +type: "LOAD_SOURCE_TEXT",
-    +sourceId: string
+    +sourceId: string,
+    +epoch: number
   |},
   Source
 >;
 export type SourceAction =
   | LoadSourceAction
   | {|
       +type: "ADD_SOURCE",
       +source: Source
--- a/devtools/client/debugger/new/src/reducers/sources.js
+++ b/devtools/client/debugger/new/src/reducers/sources.js
@@ -31,16 +31,18 @@ import { mapValues, uniqBy, uniq } from 
 export type SourcesMap = { [SourceId]: Source };
 export type SourcesMapByThread = { [ThreadId]: SourcesMap };
 
 type UrlsMap = { [string]: SourceId[] };
 type DisplayedSources = { [ThreadId]: { [SourceId]: boolean } };
 type GetDisplayedSourcesSelector = OuterState => { [ThreadId]: SourcesMap };
 
 export type SourcesState = {
+  epoch: number,
+
   // All known sources.
   sources: SourcesMap,
 
   // All sources associated with a given URL. When using source maps, multiple
   // sources can have the same URL.
   urls: UrlsMap,
 
   // For each thread, all sources in that thread that are under the project root
@@ -58,16 +60,17 @@ const emptySources = {
   sources: {},
   urls: {},
   displayed: {}
 };
 
 export function initialSourcesState(): SourcesState {
   return {
     ...emptySources,
+    epoch: 1,
     selectedLocation: undefined,
     pendingSelectedLocation: prefs.pendingSelectedLocation,
     projectDirectoryRoot: prefs.projectDirectoryRoot,
     chromeAndExtenstionsEnabled: prefs.chromeAndExtenstionsEnabled,
     focusedItem: null
   };
 }
 
@@ -159,17 +162,20 @@ function update(
         return updateSource(state, { id, isBlackBoxed });
       }
       break;
 
     case "SET_PROJECT_DIRECTORY_ROOT":
       return updateProjectDirectoryRoot(state, action.url);
 
     case "NAVIGATE":
-      return initialSourcesState();
+      return {
+        ...initialSourcesState(),
+        epoch: state.epoch + 1
+      };
 
     case "SET_FOCUSED_SOURCE_ITEM":
       return { ...state, focusedItem: action.item };
   }
 
   return state;
 }
 
@@ -291,20 +297,29 @@ function updateProjectDirectoryRoot(stat
     relativeUrl: getRelativeUrl(source, root)
   }));
 }
 
 /*
  * Update a source's loaded state fields
  * i.e. loadedState, text, error
  */
-function updateLoadedState(state, action: LoadSourceAction): SourcesState {
+function updateLoadedState(
+  state: SourcesState,
+  action: LoadSourceAction
+): SourcesState {
   const { sourceId } = action;
   let source;
 
+  // If there was a nativation between the time the action was started and
+  // completed, we don't want to update the store.
+  if (action.epoch !== state.epoch) {
+    return state;
+  }
+
   if (action.status === "start") {
     source = { id: sourceId, loadedState: "loading" };
   } else if (action.status === "error") {
     source = { id: sourceId, error: action.error, loadedState: "loaded" };
   } else {
     if (!action.value) {
       return state;
     }
@@ -517,16 +532,20 @@ export function getHasSiblingOfSameName(
 
   return getSourcesUrlsInSources(state, source.url).length > 1;
 }
 
 export function getSources(state: OuterState) {
   return state.sources.sources;
 }
 
+export function getSourcesEpoch(state: OuterState) {
+  return state.sources.epoch;
+}
+
 export function getUrls(state: OuterState) {
   return state.sources.urls;
 }
 
 export function getSourceList(state: OuterState): Source[] {
   return (Object.values(getSources(state)): any);
 }