Closed Bug 1060732 Opened 10 years ago Closed 8 years ago

JavaScript Debugger reloads sources

Categories

(DevTools :: Debugger, defect)

Product:
DevTools
Component:
Debugger
Version:
40 Branch
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox49 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: scott, Assigned: ochameau)

References

(
URL
)

Details

Attachments

(4 files, 5 obsolete files)

Screenshot from 2015-07-04 17:23:42.png
310.44 KB, image/png
Details
Page source != debugger text
600.10 KB, image/png
Details
patch v4
11.71 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
test fixes
2.19 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.94 Safari/537.36

Steps to reproduce:

After loading a webpage with inline JavaScript, open Debugger developer tool (Menu > Developer > Debugger). Select a source from the source list.


Actual results:

"Loading…" briefly appears before the JavaScript source is shown, and Firefox fetches the source from remote.


Expected results:

Debugger should use sources as they were when the page was initially loaded. Debugging certain web applications using the built-in Firefox tools isn't possible, as requesting the sources again could result in a different request, or an invalid request if there were additional request headers originally sent.
Discussed here: https://support.mozilla.org/en-US/questions/1016100
Component: Untriaged → Developer Tools: Debugger
The "Loading..." message that you see indicates that the debugger is loading the source text in the editor, not necessarily fetching the sources again. It tries to load the sources from the cache if possible, so if you are seeing unexpected results it might be that the server explicitly prevented caching. Unfortunately without more information I can't be more specific. Can you provide a link to your app to verify what is going on?
The application requires authentication and we don't have a publicly available instance unfortunately. I've included response headers for one of the pages in question below.

While I was preparing the request/response headers to add to this call I noticed that the initial request uses POST and includes some form parameters. Sure enough, the source shown in the Debugger matches the source when requesting the same page, but using GET.

Additionally, the response does request that the agent doesn't cache anything.

I'll happily accept that all of these factors are causing this issue, but Firebug does not exhibit this behaviour, and neither does Chrome's or Safari's built-in tools. Could an argument still be made that the Firefox Debugger should always use whatever is in memory?

HTTP/1.1 200 OK
Date: Mon, 01 Sep 2014 09:44:05 GMT
Server: Apache-Coyote/1.1
Cache-Control: no-store,no-cache,must-revalidate
Pragma: no-cache
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Content-Type: text/html;charset=UTF-8
Content-Length: 8462
X-Cnection: close
OK, thanks for verifying my theory. Indeed, work for using the VM's cache instead of the network cache is currently underway in bug 905700.
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → DUPLICATE
According to Scott's message in bug 905700 comment 50, that bug was not enough to resolve this issue.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
I don't know if this will help or not, but I've knocked up an example of this issue at https://afternoon-scrubland-1558.herokuapp.com/.

# Open https://afternoon-scrubland-1558.herokuapp.com/
# Open Inspector panel of developer tools
# Hit "POST" button
# Confirm presence of JavaScript in HTML script tag when viewed in Inspector
# Switch to Debugger panel of developer tools and select page source (rather than application-*.js)
# Confirm source shown by Debugger does not match page source
## Debugger panel shows GET source (for page with the "POST" button)

Another way to uncover the same annoyance:

# As above - hit "POST" button
# Inspect HTML P element on page and click little "ev" event listener icon
# Click reference to line 20 from main page
## Panel expands to show listener code
# Click "Open in Debugger" (pause icon) in panel
## Debugger panel shown but for GET source
URL: https://afternoon-scrubland-1558.hero...
Version: 31 Branch → 40 Branch
Depends on: 1118332
See Also: → 1005542
Firefox JavaScript Debugger Shows "This page has no sources."

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

=== maybe related old issues  ===
toolkit/devtools: https://bugzilla.mozilla.org/show_bug.cgi?id=1118332
LOAD_BYPASS_CACHE: https://bugzilla.mozilla.org/show_bug.cgi?id=865252

=== similar effects ===
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)
Note: Works For Me People
- Please test this out over a longer test period with more tab switching.
To be clear, "This page has no sources" isn't the topic of this bug.
(In reply to Scott Macpherson from comment #6)
> I don't know if this will help or not, but I've knocked up an example of
> this issue at https://afternoon-scrubland-1558.herokuapp.com/.

This demonstrates exactly the problem as I'm seeing it. It's no longer possible (since Firefox 39 or earlier) to place a breakpoint on inline JavaScript on a page generated by a POST request. Because the Debugger tab loads all of the page/script sources from scratch as GET requests, you no longer receive the correct page source, and any inline JavaScript specific to the page is no longer present.

I can confirm that if I right-click the page and select View Page Source, I do see the actual page source as expected.

I first discovered this in Firefox 39 in XP; I can still reproduce it in Firefox 40.0.2 in Windows 8.1 Pro (both 32-bit). It may go back before 39, as I've not needed to debug inline JavaScript in a while.

I do not recognise the message "This page has no sources."
It looks like we just have a bug around POST requests and may be other edge cases.

We have a Source object, related to the inline <script>.
And we try to fetch its source from SourceActor.url, and we use DevToolsUtils.fetch() which translate to a GET request, even if we correctly pass loadFromCache: true, it ends up creating a new GET request:

http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/source.js#349
349         let sourceFetched = fetch(this.url, { loadFromCache: this.isInlineSource });

I'm wondering if we could somehow fetch the source text from the element, instead of doing a request.
I can easily imagine other reasons why doing fetch/request would fail and/or create a new request.
Status: UNCONFIRMED → NEW
Ever confirmed: true
I don't know much the debugger actors. I'm just trying...

It would be great to just not attempt to do any request, even cached.
I'm wondering if we could just pull the sources with something like this.
By serializing the related DOM Document.
Here source.element is the <SCRIPT> tag of:
https://afternoon-scrubland-1558.herokuapp.com/?method=post
(which only exists if you did a POST request!)

Note that this patch doesn't completely fix the issue.
At least the debugger panel shows more correct sources
*BUT* breakpoints don't work perfectly:
  <script>
  $(document).ready(function(){
        $('p').click(function(){
                alert('Click!');
        }); // <=== only works if I put it there.
  });
  </script>

That's because the serializer doesn't returns the exact same output.
I'll try to look around see if we can explicitely pull the document from some cache...
Attachment #8734440 - Flags: feedback?(ejpbruel)
By the way, thanks Scott for https://afternoon-scrubland-1558.herokuapp.com/.
It helped me a ton to figure out what was wrong!

Speaking about solutions.
We may also use some components ViewSource is using to fetch the page content without redoing a request.
Do we have other bugs where the debugger do network requests whereas it should never do some?
Comment on attachment 8734440 [details] [diff] [review]
try to load sources from DOM Element. r=?

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

Thanks for trying to fix this Alex!

In general, we should avoid the use of unsafeDereference whenever possible. The purpose of unsafeDereference is to give you an escape hatch when the debugger API does not provide the functionality you need. Since you're trying to get the source text of the inline script from its DOM element, that is the case here. However, getting the source text from the DOM element shouldn't be necessary: inline sources are actual sources, and they each get their own Debugger.Source object. We should just be able to obtain the source text of an inline source from its D.S. object.

I am not sure why we're trying to fetch the source text with a GET request to begin with. It's possible that the D.S. object doesn't provide access to the source text for an inline source. If so, we should figure out why not, and whether we can extend the debugger API to support this. If that turns out to be impossible for some, we can revisit the approach you suggested here.
Attachment #8734440 - Flags: feedback?(ejpbruel) → feedback-
Oh, I see. As far as the debugger API is concerned, each inline source is a separate source. In the server, however, we represent multiple inline sources coming from the same page with a single source actor for the entire page.

The problem is that we can get the source text for each individual inline source, but that is not enough to reconstitute the source text for the page that introduced them. This is why we currently have to resort to doing a GET request.

What we need here is to extend the debugger API with a method to obtain the complete source text of the page that introduced an inline source. We already obtained that source text, otherwise the inline source would not exist.

Jim, do you know if there are any existing plans to extend the debugger API in this way?
Flags: needinfo?(jimb)
As a final remark, since the API we need for this currently does not exist (and would likely take some time to implement) using unsafeDereference as an escape hatch is an acceptable temporary solution in this case.

We could land the patch that Alex proposed as a temporary solution, provided we create a followup bug and add this as a comment to the patch (preferably with a comment explaining why all this is necessary).
I think it is better to treat each inline source as a separate source; trying to recombine them has always led to lots of bugs.
(In reply to Eddy Bruel [:ejpbruel] from comment #16)
> What we need here is to extend the debugger API with a method to obtain the
> complete source text of the page that introduced an inline source. We
> already obtained that source text, otherwise the inline source would not
> exist.
> 
> Jim, do you know if there are any existing plans to extend the debugger API
> in this way?

No. My understanding is that we can't afford the memory required to keep around the full text of the page, and then treat the individual sources as substrings of that. We can afford the memory to keep around the original JS text because we compress it, and because lazy function compilation pays for it.

Debugger.Source instances were designed to provide enclosingStart and lineCount properties to help relate the fragment to the larger document, if it can be obtained. Those could be implemented, if we had a reliable way to fetch the original.

At the implementation level, Debugger.Source instances reflect individual compilation requests from the embedding to SpiderMonkey. Since Gecko itself just passes the scripts to SpiderMonkey one at a time as they're inserted into the DOM, that's the way we represent them.

(It was the case, at one point, that Chrome's JS debugger presented each script element in isolation. Now they present it in context, which is clearly the better approach.)
Flags: needinfo?(jimb)
Sorry but I don't really follow all your discussions. But It is quite terrifying that we don't support debugging pages using POST, which seems to be the original issue behind this bug :/

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #18)
> I think it is better to treat each inline source as a separate source;
> trying to recombine them has always led to lots of bugs.

It sounds like you are against the current behavior, right?

I'm not sure I can easily do such behavior change. But I can surely tweak my existing patch, which just fixes things around existing practices.

(In reply to Jim Blandy :jimb from comment #19)
> (It was the case, at one point, that Chrome's JS debugger presented each
> script element in isolation. Now they present it in context, which is
> clearly the better approach.)

Sounds like you are saying we should keep doing what we do?


Otherwise, my patch is still wrong as the DOMSerializer isn't bijective.
I would like to look into what view-source: is doing. It is pulling content from cache in a very special way.
Do you know in which other cases we end up calling fetch over here?
http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/source.js#343
349         let sourceFetched = fetch(this.url, { loadFromCache: this.isInlineSource });

I imagine other cases would also need such fix anyway.
(In reply to Alexandre Poirot [:ochameau] from comment #20)
> (In reply to Jim Blandy :jimb from comment #19)
> > (It was the case, at one point, that Chrome's JS debugger presented each
> > script element in isolation. Now they present it in context, which is
> > clearly the better approach.)
> 
> Sounds like you are saying we should keep doing what we do?

It seems like the preferable behavior, if we have it implemented somehow.
Attached patch patch v2 (obsolete) — — Splinter Review 
This patch is much better. It correctly fetch sources from cache this time.
It still involves unsafeDereference, but I don't think that's such an issue
as we use it sooner in TabSources.createNonSourceMappedActor,
to compute the isInlineSource field:
http://mxr.mozilla.org/mozilla-central/source/devtools/server/actors/utils/TabSources.js#298
298     // Assume the source is inline if the element that introduced it is not a
299     // script element, or does not have a src attribute.
300     let element = aSource.element ? aSource.element.unsafeDereference() : null;
301     if (element && (element.tagName !== "SCRIPT" || !element.hasAttribute("src"))) {
302       spec.isInlineSource = true;

This new usage is directly related to this first usage!

May be we could compute principal and cache key right here and pass it along up to getSourceText?
That would allow me to *not delete* source in TabSource.source function.
(This is a workaround, that needs to be fixed somehow, that allows me to have access to source.element
from getSourceText)

With that fixed and a test, I'm ready to ask review for this patch.

Again, are you aware of some other bugs related to the debugger doing or not doing http requests??
I wouldn't be surprise if that patch fixes other edge cases!!
Attachment #8736330 - Flags: feedback?(ejpbruel)
Attachment #8734440 - Attachment is obsolete: true
Comment on attachment 8736330 [details] [diff] [review]
patch v2

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

I'm not sure if you intended to f? or r?.

In any case, I've added two comments. If you address those and r? again, I *should* be able to r+ next round.

::: devtools/server/actors/source.js
@@ +339,5 @@
>             this._contentType === "text/wasm")) {
>          return toResolvedContent(this.source.text);
>        }
>        else {
> +        let principal, cacheKey, loadFromCache = this.isInlineSource;

Please don't mix declarations with initialisation.

This code can be misleading if you're not paying attention, especially since let { principal, cacheKey, loadFromCache } = this.isInlineSource; is also legal and does something completely different.

@@ +341,5 @@
>        }
>        else {
> +        let principal, cacheKey, loadFromCache = this.isInlineSource;
> +        // Try to fetch the sources from the related DOM Element if available
> +        let element = this.source && this.source.element ? this.source.element.unsafeDereference() : null;

I'm ok with you using unsafeDereference here. However, I want to make clear that the fact that we're already using unsafeDereference somewhere else in itself is not enough reason to justify its use here:

unsafeDereference() is intended as a stopgap measure for those cases where the debugger API does not provide the functionality we need. In general, our goal should be to extend the debugger API so that we can remove existing uses of unsafeDereference. We should not add uses of unsafeDereference unless it's absolutely necessary.

That said, I think this is a clear case where the debugger API does not give you a way to do what you need, so using unsafeDereference here as a temporary solution is acceptable. We should still open a followup bug to extend the debugger API so you can obtain the HTML source directly, and refer to that bug in a comment here.
Attachment #8736330 - Flags: feedback?(ejpbruel) → feedback+
Hi Alex,

I'd like to follow up on our last conversation on irc. I just had a talk with Jim about your patch. He agrees with your point that unsafeDereference is fine whenever you need to access Gecko specific things. The debugger only has knowledge about the JavaScript Engine. It can not, and should not have any knowledge about Gecko.

That means that my point of view on the use of unsafeDereference was too narrow. We should still avoid it whenever possible, and in some cases we really should try to extend the debugger API instead, but in cases such as these, using unsafeDereference is perfectly fine.

With that in mind, you don't have to file a followup bug, or add the todo comment I requested earlier.

I also had a question. I just realised that you are fetching the HTML source text directly from the cache. Is it not possible to obtain the source text directly from the element? What is the reason we have to fetch it from the cache? I do not disagree with your approach, but I would like to understand why you're doing it this way).
Flags: needinfo?(poirot.alex)
If the site doesn't permit pages to be cached, what implications does that have here?
(In reply to Eddy Bruel [:ejpbruel] from comment #25)
> Hi Alex,
> 
> I'd like to follow up on our last conversation on irc. I just had a talk
> with Jim about your patch.

Great, thanks!

> I also had a question. I just realised that you are fetching the HTML source
> text directly from the cache. Is it not possible to obtain the source text
> directly from the element? What is the reason we have to fetch it from the
> cache? I do not disagree with your approach, but I would like to understand
> why you're doing it this way).

We don't want just the script element text here but the whole document sources, with HTML and all inline scripts.
I first tried to use DOMSerializer, but it isn't bijective. It doesn't serialize back exactly to what we downloaded from http. I already tried document.documentElement.outerHTML, it seems mostly good except that it misses the DOCTYPE declaration :/
Tweaking the fetch from cache what the only working solution I found. Which is a bit similar to what is done in view source.


(In reply to Daniel Beardsmore from comment #26)
> If the site doesn't permit pages to be cached, what implications does that
> have here?

Good catch! I'm not sure it will work. For ressources using GET it will surely just redo the request, but it may fail again for POST requests. Do you have a more precise case in mind with particular http header or something? Or a online test case I could use to verify the patch?
I don't have a test case for this situation; it just concerned me because I know that many sites do enforce no-cache.
Assignee: nobody → poirot.alex
Flags: needinfo?(poirot.alex)
Attached patch patch v3 (obsolete) — — Splinter Review 
With a test. Can you please carefully review the test?
I copied some existing practices, but it looks like:
  let [tab,, panel] = yield initDebugger(TAB_URL);
  ...
  yield waitForSourceShown(panel, TAB_URL);
introduce some timeout intermittent where waitForSourceShown never resolves.
It looks like the source shown event is dispatched during the call to initDebugger,
but I can't listen for the even before calling initDebugger as I need panel...
And I have to wait for the source otherwise it isn't there in most runs.

Otherwise I think you will prefer this patch over the previous ones.
I no longer use unsafeDereference. In fact, at the end, I don't need the element,
but the related document we have to load. And I can get it through threadActor's window object.

In this new patch I also fixed try to get it green when running on xpcshell and workers.

I'll verify the no-cache story on monday, but I think we can already iterate with this fix
which clearly address the original report, and possibly many other issues!!

Always loading via the system principal looks wrong, I tuned that to use the document principal.
The cache key ensure reloading the exact same cache used for the current tab.
If we load the same URL in two tabs we will just load whatever has been loaded last.
Attachment #8737144 - Flags: review?(ejpbruel)
Attachment #8736330 - Attachment is obsolete: true
Comment on attachment 8737144 [details] [diff] [review]
patch v3

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

The general approach looks fine to me. The main reason I r-'d is because I think the patch should be better commented.

I hope you don't think I'm being difficult about this: my experience is that not everyone hacking on the debugger is as knowledgeable about Gecko as someone like you. Within a couple of months, nobody will remember why this code is here, or why it was written the way it is. That in turn makes it very hard to refactor, without introducing unanticipated regressions. With that in mind, I hope you'll understand why I'm pushing for more comments.

::: devtools/client/debugger/test/mochitest/browser_dbg_post-page.js
@@ +19,5 @@
> +  let editor = win.DebuggerView.editor;
> +  let queries = win.require('./content/queries');
> +  let getState = win.DebuggerController.getState;
> +
> +  yield waitForSourceShown(panel, TAB_URL);

Like you said, this looks racy. What happens if the source is already shown before we wait for it?

I'm not as knowledgeable about the frontend of the debugger as I am about the backend, so let's forward this question to James.

::: devtools/server/actors/source.js
@@ +134,5 @@
>   *        Optional. For sourcemapped urls, the original url this is representing.
>   * @param Debugger.Source generatedSource
>   *        Optional, passed in when aSourceMap is also passed in. The generated
>   *        source object that introduced this source.
> + * @param String isInlineSource

This should be a Boolean.

@@ +135,5 @@
>   * @param Debugger.Source generatedSource
>   *        Optional, passed in when aSourceMap is also passed in. The generated
>   *        source object that introduced this source.
> + * @param String isInlineSource
> + *        Optional. True if is a inlined script from a HTML or XUL page.

"if this is an inline source from"

@@ +341,5 @@
>             this._contentType === "text/wasm")) {
>          return toResolvedContent(this.source.text);
>        }
>        else {
> +        let loadFromCache = this.isInlineSource;

This needs an explanation. Why do we load the source text from the cache if it's an inline source? (If you don't feel like writing a lengthy explanation, you can just link to the bug if you want. Adding a short explanation would be better though).

@@ +347,5 @@
> +        // Fetch the sources with the same principal than the original document
> +        let win = this.threadActor._parent.window;
> +        let principal, cacheKey;
> +        // On xpcshell, we don't have a window but a Sandbox
> +        if (!isWorker && win instanceof Ci.nsIDOMWindow) {

We only want to load from the cache if:
1. This is an inline source.
2. We are not in a worker.
3. win is an instance of nsIDOMWindow (we are not in an xpcshell)

In my opinion, this code would be easier to read if we made the value of loadFromCache that of the predicate above, and then make this entire block of code conditional on the value of loadFromCache.

@@ +354,5 @@
> +          let channel = webNav.currentDocumentChannel;
> +          principal = channel.loadInfo.loadingPrincipal;
> +
> +          // Retrieve the cacheKey in order to load POST requests from cache
> +          if (loadFromCache && webNav.currentDocumentChannel instanceof Ci.nsICacheInfoChannel) {

Why does currentDocumentChannel have to be an instance of Ci.nsICacheInfoChannel for us to be able to obtain a cacheKey for it? And why is it possible for currentDocumentChannel *not* to be an instance of Ci.nsICacheInfoChannel?

This definitely needs a comment.

@@ +366,5 @@
>          // fetching the original text for sourcemapped code, and the
>          // page hasn't requested it before (if it has, it was a
>          // previous debugging session).
> +        let sourceFetched = fetch(this.url, {
> +          policy: isWorker ? null : Ci.nsIContentPolicy.TYPE_DOCUMENT,

This code looks a bit suspicious. We can have three cases here:
1. loadFromCache is false, and cacheKey is undefined.
2. loadFromCache is true, and cacheKey is webNav.currentDocumentChannel.cacheKey.
3. loadFromCache is true, and cacheKey is undefined.

The first two cases are reasonable, but I'm not sure what will happen in the third case. Will fetch simply not use the cache in that case? If so, please add a comment here explaining that.

In any case, I think loadFromCache should not be true unless we actually try to obtain a cacheKey (see comment above).

::: devtools/shared/DevToolsUtils.js
@@ +411,5 @@
>    channel.loadFlags = aOptions.loadFromCache
>      ? channel.LOAD_FROM_CACHE
>      : channel.LOAD_BYPASS_CACHE;
>  
> +  if (aOptions.cacheKey && channel instanceof Ci.nsICacheInfoChannel) {

Why does channel have to be an instance of Ci.nsICacheInfoChannel for us to be able to use the cacheKey? This needs a comment.

@@ +412,5 @@
>      ? channel.LOAD_FROM_CACHE
>      : channel.LOAD_BYPASS_CACHE;
>  
> +  if (aOptions.cacheKey && channel instanceof Ci.nsICacheInfoChannel) {
> +    channel.cacheKey = aOptions.cacheKey;

It looks like we are using cacheKey here regardless of the value of loadFromCache. The comment above implies that cacheKey is only used when we're loading from cache (i.e. if loadFromCache is set to true). Please change the code to reflect this.

Also, it looks like it is possible for loadFromCache to be set to true, while cacheKey is set to undefined. Should we still assign the cacheKey to the channel in that case? This probably isn't a problem, but it would be nice to add a comment here to remind us that cacheKey can be undefined.
Attachment #8737144 - Flags: review?(ejpbruel) → review-
James, can you take a quick look at the test in Alex's patch and confirm whether the way we are using waitForSource is racy or not? If it is, how should this code be rewritten?
Flags: needinfo?(jlong)
Attached patch interdiff (obsolete) — — Splinter Review 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v4
        — 
          — Splinter Review
      

    


  
See previous attachment for interdiff.

(In reply to Eddy Bruel [:ejpbruel] from comment #31)
> Comment on attachment 8737144 [details] [diff] [review]
> patch v3
> 
> Review of attachment 8737144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The general approach looks fine to me. The main reason I r-'d is because I
> think the patch should be better commented.
> 
> I hope you don't think I'm being difficult about this:

Ok, you used "should", you are fine ;)

  > If you address those and r? again, I *should* be able to r+ next round.


> ::: devtools/client/debugger/test/mochitest/browser_dbg_post-page.js
> @@ +19,5 @@
> > +  let editor = win.DebuggerView.editor;
> > +  let queries = win.require('./content/queries');
> > +  let getState = win.DebuggerController.getState;
> > +
> > +  yield waitForSourceShown(panel, TAB_URL);
> 
> Like you said, this looks racy. What happens if the source is already shown
> before we wait for it?
> 
> I'm not as knowledgeable about the frontend of the debugger as I am about
> the backend, so let's forward this question to James.

Yes, one try run failed with such race, but I don't get it, existing tests are already doing that:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/debugger/test/mochitest/browser_dbg_auto-pretty-print-01.js#18

> @@ +341,5 @@
> >             this._contentType === "text/wasm")) {
> >          return toResolvedContent(this.source.text);
> >        }
> >        else {
> > +        let loadFromCache = this.isInlineSource;
> 
> This needs an explanation. Why do we load the source text from the cache if
> it's an inline source? (If you don't feel like writing a lengthy
> explanation, you can just link to the bug if you want. Adding a short
> explanation would be better though).

There was already a good explanation a few lines after, I moved it here.

> @@ +347,5 @@
> > +        // Fetch the sources with the same principal than the original document
> > +        let win = this.threadActor._parent.window;
> > +        let principal, cacheKey;
> > +        // On xpcshell, we don't have a window but a Sandbox
> > +        if (!isWorker && win instanceof Ci.nsIDOMWindow) {
> 
> We only want to load from the cache if:
> 1. This is an inline source.
> 2. We are not in a worker.
> 3. win is an instance of nsIDOMWindow (we are not in an xpcshell)
> 
> In my opinion, this code would be easier to read if we made the value of
> loadFromCache that of the predicate above, and then make this entire block
> of code conditional on the value of loadFromCache.

There is two things in this block. The principal and the cacheKey.
The principal is correctly set also for request that are not fetched from cache.
Before this patch we were always using the system principal, which is just bad.
There is no reason to use the right principal only for cached requests.

> @@ +354,5 @@
> > +          let channel = webNav.currentDocumentChannel;
> > +          principal = channel.loadInfo.loadingPrincipal;
> > +
> > +          // Retrieve the cacheKey in order to load POST requests from cache
> > +          if (loadFromCache && webNav.currentDocumentChannel instanceof Ci.nsICacheInfoChannel) {
> 
> Why does currentDocumentChannel have to be an instance of
> Ci.nsICacheInfoChannel for us to be able to obtain a cacheKey for it? And
> why is it possible for currentDocumentChannel *not* to be an instance of
> Ci.nsICacheInfoChannel?
> 
> This definitely needs a comment.

Hum. Did you knew that instanceof was equivalent of doing a QueryInterface that succeed?
I'm using it in that way, which is somewhat common.
It is somewhat equivalent of:
if (loadFromCache) {
  webNav.currentDocumentChannel.QueryInterface(Ci.nsICacheInfoChannel);
  cacheKey = webNav.currentDocumentChannel.cacheKey;
}
I do have to QI, otherwise cacheKey isn't defined on this object.
cacheKey is part of this interface:
  http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsICacheInfoChannel.idl#30

I switched to the above code, hopefully it is clearer.

> 
> @@ +366,5 @@
> >          // fetching the original text for sourcemapped code, and the
> >          // page hasn't requested it before (if it has, it was a
> >          // previous debugging session).
> > +        let sourceFetched = fetch(this.url, {
> > +          policy: isWorker ? null : Ci.nsIContentPolicy.TYPE_DOCUMENT,
> 
> This code looks a bit suspicious. We can have three cases here:
> 1. loadFromCache is false, and cacheKey is undefined.
> 2. loadFromCache is true, and cacheKey is
> webNav.currentDocumentChannel.cacheKey.
> 3. loadFromCache is true, and cacheKey is undefined.
> 
> The first two cases are reasonable, but I'm not sure what will happen in the
> third case. Will fetch simply not use the cache in that case? If so, please
> add a comment here explaining that.
> 
> In any case, I think loadFromCache should not be true unless we actually try
> to obtain a cacheKey (see comment above).

cacheKey should always be set. currentDocumentChannel should always implement nsICacheInfoChannel.
But I added a JS assert if that can help chase uncommon behavior.


> ::: devtools/shared/DevToolsUtils.js
> @@ +411,5 @@
> >    channel.loadFlags = aOptions.loadFromCache
> >      ? channel.LOAD_FROM_CACHE
> >      : channel.LOAD_BYPASS_CACHE;
> >  
> > +  if (aOptions.cacheKey && channel instanceof Ci.nsICacheInfoChannel) {
> 
> Why does channel have to be an instance of Ci.nsICacheInfoChannel for us to
> be able to use the cacheKey? This needs a comment.

Same thing than previous comment. It is equivalent of QI.
Do you want me to also switch to:
  if (aOption.cacheKey) {
    channel.QueryInterface(Ci.nsICacheInfoChannel);
    channel.cacheKey = aOption.cacheKey;
  }
?

> @@ +412,5 @@
> >      ? channel.LOAD_FROM_CACHE
> >      : channel.LOAD_BYPASS_CACHE;
> >  
> > +  if (aOptions.cacheKey && channel instanceof Ci.nsICacheInfoChannel) {
> > +    channel.cacheKey = aOptions.cacheKey;
> 
> It looks like we are using cacheKey here regardless of the value of
> loadFromCache. The comment above implies that cacheKey is only used when
> we're loading from cache (i.e. if loadFromCache is set to true). Please
> change the code to reflect this.
> 
> Also, it looks like it is possible for loadFromCache to be set to true,
> while cacheKey is set to undefined. Should we still assign the cacheKey to
> the channel in that case? This probably isn't a problem, but it would be
> nice to add a comment here to remind us that cacheKey can be undefined.

Good point, note that the arguments documentation already states that is only
works when loading from cache.
I now only set the cacheKey if loadFromCache is true.
Note that setting cacheKey without setting loadFlags to LOAD_FROM_CACHE
doesn't force loading from cache, it will just do a regular request.

        Attachment #8738540 -
        Flags: review?(ejpbruel)

    
  

        Attachment #8737144 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Eddy Bruel [:ejpbruel] from comment #32)
> James, can you take a quick look at the test in Alex's patch and confirm
> whether the way we are using waitForSource is racy or not? If it is, how
> should this code be rewritten?

I've never loved how that works, it's how a lot of our tests are written. I think it technically could be racy, but it's such a rare case that it hasn't ever come up. That code runs right after `initDebugger` is resolved which is right after the panel loads. Initially no source is shown, and after a small delay (200ms) it randomly selects an initial source. Because of that delay we've never hit an issue. There's no way that the promise from the panel loading is going to resolve so slow that it's after the first source is shown.

I agree it's not ideal, but we're stuck with it for now. I'd write the test that way because many of our other tests are like it. We are currently working on a rewrite of the debugger and that will come with a whole new test suite.
Flags: needinfo?(jlong)

    
    

    
  
Comment on attachment 8738540 [details] [diff] [review]
patch v4

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

Thanks for adding the comments I requested. I just have a few more nits.

I also requested you restructure the code in source.js somewhat. You didn't address this comment. Please do so before landing this patch.

::: devtools/server/actors/source.js
@@ +347,5 @@
>          // 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).
> +        let loadFromCache = this.isInlineSource;

In my previous review, I requested that you write this code like this instead:

let loadFromCache = this.isInlineSource &&
                    !isWorker &&
                    win instance of Ci.nsIDOMWindow;

And then rewrite the two if-checks here below into a single if-check.

@@ +349,5 @@
>          // page hasn't requested it before (if it has, it was a
>          // previous debugging session).
> +        let loadFromCache = this.isInlineSource;
> +
> +        // Fetch the sources with the same principal than the original document

same principal as

::: devtools/shared/DevToolsUtils.js
@@ +378,5 @@
>   *        - charset: the charset to use if the channel doesn't provide one
> + *        - principal: the principal to use, if omitted, the request is loaded
> + *                     with the system principal
> + *        - cacheKey: when loading from cache, use this key to retrieve a cache
> + *                    specific to a given SHEntry. (Allows to load POST

Allows loading

@@ +411,5 @@
>    channel.loadFlags = aOptions.loadFromCache
>      ? channel.LOAD_FROM_CACHE
>      : channel.LOAD_BYPASS_CACHE;
>  
> +  // When loading from cache, the cacheKey allows to target a specific SHEntry

allows us to

        Attachment #8738540 -
        Flags: review?(ejpbruel) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        interdiff (obsolete)
        — 
          — Splinter Review
      

    


  
Interdiff with previous r+ patch.
Fixes comments, but not the if condition.

I also had to revert back to `webNav.currentDocumentChannel instanceof Ci.nsICacheInfoChannel`
in order to support chrome documents.
XUL Documents loaded via chrome:// URIs don't support the nsICacheInfoChannel interface,
so there is no point in trying to fetch cacheKey.
(The QueryInterface from the previous patch was throwing)

> ::: devtools/server/actors/source.js
> @@ +347,5 @@
> >          // 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).
> > +        let loadFromCache = this.isInlineSource;
> 
> In my previous review, I requested that you write this code like this
> instead:
> 
> let loadFromCache = this.isInlineSource &&
>                     !isWorker &&
>                     win instance of Ci.nsIDOMWindow;
> 
> And then rewrite the two if-checks here below into a single if-check.

I already replied about this, I'm not trying to silently not address something!
This `if (!isWorker && win instanceof Ci.nsIDOMWindow) {` block isn't only about cache.
It also tries to fetch a principal.
Doing what you suggest would fetch all sources whose isInlineSource is false
via system principal. Whereas with my current patch it will fetch all resources
that are related to a document with the document principal *and* optionaly,
if isInlineSource is true and their document support the cache interface,
will fetch the very precise cached version from SHEntry.

        Attachment #8740450 -
        Flags: review?(ejpbruel)

    
  

        Attachment #8738538 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8740450 [details] [diff] [review]
interdiff

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

Hi Alex.

I wasn't implying that you were silently not addressing something. Sorry if I gave that impression! I assumed that you had either forgotten to address my comment, or had a good reason for writing the code the way you did. As it turns out, the latter is the case.

With your explanation, this code now makes sense to me, so go ahead and land it!

        Attachment #8740450 -
        Flags: review?(ejpbruel) → review+

    
    

    
  
https://hg.mozilla.org/integration/fx-team/rev/aa1a374f6bde61d7e39179bc95dadd1793b9b3f7
Bug 1060732 - Load sources from page loaded via POST from cache in the debugger. r=ejpbruel

    
    

    
  

      
      
      
      

        Attached patch
          
          
        test fixes
        — 
          — Splinter Review
      

    


  
Looks like initDebugger really races as unexpected ;)
I have no idea why my test is any different from others...

I also had to revert a change about policy.
In my previous patch, while trying to fix the principal,
I was also trying to set the correct content policy,
but it raises assertions in other tests.

With this, I finally got a green try on windows!

        Attachment #8743944 -
        Flags: review?(ejpbruel)

    
  

        Attachment #8740450 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Alexandre Poirot [:ochameau] from comment #53)
> Looks like initDebugger really races as unexpected ;)

See also: bug 1080025

    
  
Flags: needinfo?(poirot.alex)

    
    

    
  
Comment on attachment 8743944 [details] [diff] [review]
test fixes

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

Patch looks good to me.

        Attachment #8743944 -
        Flags: review?(ejpbruel) → review+

    
    

    
  
https://hg.mozilla.org/integration/fx-team/rev/de6bfcdfe5ce1f7113e085147d228e4b19de456c
Bug 1060732 - Load sources from page loaded via POST from cache in the debugger. r=ejpbruel

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/de6bfcdfe5ce
Status: NEW → RESOLVED
Closed: 10 years ago → 8 years ago

          status-firefox49:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

    
  
Depends on: 1273311

    
    

    
  
Reproduced this bug on Nightly 41.0a1 (2015-05-29) {Build ID: 20150529030205) by following Comment 6 instruction on Linux.

This Bug's Fix is now verified on Latest Firefox Beta 49.0b3

Build ID: 20160811031722
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
OS: Linux 4.4.0-2-deepin-amd64
QA Whiteboard: [testday-20160812]

    
    

    
  
I am using filrefox 43, os win 7 and the problem exist occasional.
I am developing joomla components, and often when I open javascript debugger, instead of seeing the actual page source,  debugger shows me the source of login page. Always, when this issue happens, it shows me the login page source, no matter what is the page I am into.
Also until now I have notice this only when I am trying to debug scripts that exists inside the page and not when I am trying to debug scripts that have been imported to page through <script src="xxxx"></script>.

    
  
Product: Firefox → DevTools

    
    

    
  
I'm using Firefox Quantum 69.0b14 and for me the problem still exists.

In my application I load the complete javascript from a single url ("/js/app"). The source is build depending on the rights the user has for the application.

Looking at the my internal webserver protocoll I see that the application load the javascript and then there is another request for "/js/app" without session cookie header and so the response is empty.

Is there any setting I have to choose?

In the meantime I have to switch to another browser without this problem. Sorry! I love Firefox, but with this issue I can't debug my application.



    
    

    
  
I'm sorry for the issue. I believe we actually fixed this a couple of weeks ago in our Nightly branch which will go to beta in a week. Would you mind testing it and letting us know if it works for you?


https://www.mozilla.org/en-US/firefox/channel/desktop/



    
    

    
  
In Firefox 71.0b3 the problem still exists.



    
  
See Also:  → 1890697

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: