Closed Bug 1118332 Opened 5 years ago Closed 4 years ago

Only load the HTML page from browser cache in the debugger

Categories

(DevTools :: Debugger, defect)

x86
macOS
defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: jlong, Assigned: jlong)

References

Details

(Whiteboard: [polish-backlog])

Attachments

(1 file, 1 obsolete file)

When loading a script's source, if we can't use the Debugger.Source text (sourcemapped, etc) we try to load it from the cache to skip another GET request. However we can't load it from cache all the time, for example we don't know when sourcemapped sources are updated.

I'm a little confused about our rules for this and the code isn't clear to me, so I want to make sure it's working correctly after the big Debugger.Source change. Right now in `_getSourceText` it roughly follows this steps:

1. if there's a sourcemap and it has the source contents for my url, return it
2. if I have a Debugger.Source object, and my contentType is javascript, return the `text` property of the D.S object
3. otherwise, do a GET request of my url, loading from cache if I do NOT have a Debugger.Source object

For #3, we hit that when the source is sourcemapped or it's the special source representing the HTML page. Both don't have a Debugger.Source property or a contentType. The loading from cache logic here feels wrong, as the comment implies we should only load from the cache if it's not sourcemapped:

        // XXX bug 865252: Don't load from the cache if this is a source mapped
        // source because we can't guarantee that the cache has the most up to date
        // content for this source like we can if it isn't source mapped.
        let sourceFetched = fetch(this.url, { loadFromCache: !this.source });

Since we load the source from the Debugger.Source.prototype.text property now, I can't even remember when we get to this point and we *can* load from cache. When is that even possible? If we are here, we either don't have a source or the contentType is not JS, which would always imply sourcemapped right? Is there a time when we DO have a Debugger.Source object but the contentType is NOT javascript?
This bug is a continuation from https://bugzilla.mozilla.org/show_bug.cgi?id=905700#c57
Flags: needinfo?(nfitzgerald)
> Since we load the source from the Debugger.Source.prototype.text property now,
> I can't even remember when we get to this point and we *can* load from cache.

We do not load from D.S.p.t for an HTML page and the source actor will not have `this.source`, but the cache should have the HTML page (and the most up to date version with respect to the page we are debugging) because the browser _just_ requested it in order to display it / run JS / etc.

We cannot rely on the cache for source mapped sources because the browser will generally never request the original source files unless the debugger is open. Even if it was in the cache, it is from an old debugging session (and therefore can't be trusted to be up to date), because the browser doesn't request these files automatically, unlike non source mapped sources, or HTML pages, etc.
Flags: needinfo?(nfitzgerald)
Assignee: nobody → jlong
Whiteboard: [devedition-40]
Renaming because I figured out what we need to do
Summary: Investigate loading source from cache behavior → Only load the HTML page from browser cache in the debugger
Attached patch 1118332.patch (obsolete) — Splinter Review
The old code always felt a little wrong to me, and I think it was. I think I made it so that getting the original contents of a sourcemapped source *was* loading from cache again, which is bad. It was `loadFromCache: !this.source` which means for sourcemapped sources (which don't have `source`) it would load from cache. We made it this was in the first place because the HTML page did not have a source, so this told it to load the HTML page from the cache.

I made it a lot more explicit and easy to reason about. Sources are now explicitly marked as an inline source (basically, the SourceActor represents the HTML page) and we only load from cache is it is an inline source.
Attachment #8598213 - Flags: review?(nfitzgerald)
Comment on attachment 8598213 [details] [diff] [review]
1118332.patch

Review of attachment 8598213 [details] [diff] [review]:
-----------------------------------------------------------------

Great change, thanks!

::: toolkit/devtools/server/actors/script.js
@@ +2433,5 @@
> +        // there are inline sources). Otherwise, we can't trust the
> +        // cache because we are most likely here because we are
> +        // fetching the original text for sourcemapped code, and the
> +        // page hasn't requested it before (if it has, it was a
> +        // previous debugging session)

Super nit: period
Attachment #8598213 - Flags: review?(nfitzgerald) → review+
Attached patch 1118332.patchSplinter Review
comments addressed
Attachment #8598213 - Attachment is obsolete: true
asan build seem broken, saw that in other bugs too
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/74ce17691f86
Whiteboard: [devedition-40] → [devedition-40][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/74ce17691f86
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][fixed-in-fx-team] → [devedition-40]
Target Milestone: --- → Firefox 40
Blocks: 1060732
Firefox JavaScript Debugger Shows "This page has no sources."

=== special comment ===
Confirmed in Release and Nightly 42 Build (7/3/2015) (Linux + OSX).

=== screenshot ===
https://bugzilla.mozilla.org/attachment.cgi?id=8629607

=== similar effects ===
https://bugzilla.mozilla.org/show_bug.cgi?id=1013547
https://bugzilla.mozilla.org/show_bug.cgi?id=1060732
https://bugzilla.mozilla.org/show_bug.cgi?id=927673
https://bugzilla.mozilla.org/show_bug.cgi?id=1146161
https://bugzilla.mozilla.org/show_bug.cgi?id=984645

=== possible cause ===
/toolkit/devtools/server/actors/script.js

_getSourceText: function () 

  if (this.source &&
      this.source.text !== "[no source]" &&
      this._contentType &&
      this._contentType.indexOf('javascript') !== -1) {
    return toResolvedContent(this.source.text);
  }

=== questions ===
1. What actually displays "This page has no sources."
2. Is there anyway to simplify the if() and just always display the source?
3. Is it possible that the else() case is the cause; meaning the source is detected it just doesn't display?
4. Is there anyway to write a test case? (test_sourcemaps)
Flags: needinfo?(jlong)
Please file a new bug!
Flags: needinfo?(jlong)
Whiteboard: [devedition-40] → [polish-backlog]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.