Bug 1534847 - Part 2: Invalidate the loadSourceText cache on navigation. r=jlast
authorLogan Smyth <loganfsmyth@gmail.com>
Thu, 14 Mar 2019 14:35:30 -0400
changeset 522010 35af4e6b563343968f5556c7aebdabc929553c1d
parent 522009 3befa22661a8cbab7b30f337fb3b42fbcb30fcdd
child 522011 f3500b0a2f3bbce27476b4602d1e7be26524d909
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 Summary: 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. Reviewers: jlast Bug #: 1534847 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);
 }