Closed Bug 1158498 Opened 9 years ago Closed 9 years ago

Can't set a JS breakpoint in inline JS

Categories

(DevTools :: Debugger, defect)

Product:
DevTools
Component:
Debugger
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox40 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: hub, Assigned: ejpbruel)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Screenshot from 2015-04-28 10-02-59.png
178.51 KB, image/png
Details
breakpoint-sliding.gif
98.97 KB, image/gif
Details
Use Debugger.Source.prototype.element to detect whether a source is inline
2.92 KB, patch
jlong
: review+
Details | Diff | Splinter Review 
Can't set a JavaScript breakpoint in inline JS that is enclosed in <!-- -->

When I try to set it sets it to the next inline JS not enclosed in HTML comments.
Component: Developer Tools → Developer Tools: Debugger
Can you post code so that we can see exactly what you're talking about?
It seems that the bug is more subtle than that.

(You might want to make sure Flash is completely disabled.)

Go to this page:

http://www.serenahphotography.com.au/html_ver/

Click at the bottom on portfolio.
Open the Debugger.
Try to set a breakpoint at line 142. It get set at 1206 instead.

Note: this doesn't happen on the top level URL open in a *new* tab. But navigating away and back ("Home" link) seems to do the trick.
This is not a bug. `<!--` and `-->` are HTML comments and everything inside them is stripped out no matter what, even in a script tag. See this:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8">
    <script>
      <!-- alert('bar'); -->
    </script>
  </head>
  <body>
  </body>
</html>

Nothing happens.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
(In reply to James Long (:jlongster) from comment #3)
> This is not a bug. `<!--` and `-->` are HTML comments and everything inside
> them is stripped out no matter what, even in a script tag. See this:
> 
> <!DOCTYPE html>
> <html>
>   <head>
>     <meta charset="UTF-8">
>     <script>
>       <!-- alert('bar'); -->
>     </script>
>   </head>
>   <body>
>   </body>
> </html>
> 
> Nothing happens.

Then explain how the code is actually run - because it is. And how the breakpoint actually work the first time.
Note: the document in question has no doctype, hence it runs in quirks mode.
Reopening.

This:

<html>
  <head>
    <meta charset="UTF-8">
    <script>
      <!-- 
      alert('bar'); 
      -->
    </script>
  </head>
  <body>
  </body>
</html>


Works.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Because the code is on a different line, it is run. This has always been the case and was originally designed that way for browser that didn't understand the <script> tag (hello the 90's).

Even with a doctype it does it.

But this does not help reproducing the original bug.
Ah, you're right. Basically a breakpoint is moved when it checks for a script in the system at that location but none exists. There's a couple things at play here that could be the cause and we'll look into it soon.
Blocks: dbg-breakpoint
(In reply to Hubert Figuiere [:hub] from comment #2)
> It seems that the bug is more subtle than that.
> 
> (You might want to make sure Flash is completely disabled.)
> 
> Go to this page:
> 
> http://www.serenahphotography.com.au/html_ver/
> 
> Click at the bottom on portfolio.
> Open the Debugger.
> Try to set a breakpoint at line 142. It get set at 1206 instead.
> 
> Note: this doesn't happen on the top level URL open in a *new* tab. But
> navigating away and back ("Home" link) seems to do the trick.

I tried to reproduce this, but I seem to be missing something. The only source file I see is flashobject.js, which is not a HTML file, and doesn't have a line 1206. What am I missing?
Flags: needinfo?(hub)
See the attached screenshot. "gallery.php" is the source file.
Flags: needinfo?(hub)
Because that's inline Javascript.
(In reply to Hubert Figuiere [:hub] from comment #11)
> Because that's inline Javascript.

Gotcha. I still cannot reproduce the issue exactly like you said. Setting the breakpoint on line 142 in that file seems to work (it does not slide down to line 1206). However, if I then refresh the page, or navigate back and forth, the breakpoint is never hit.

What exact steps did you use to make the breakpoint slide to line 1206?
Flags: needinfo?(hub)
Attached image breakpoint-sliding.gif — 
Here's what I see:

Open this in a new tab: http://www.serenahphotography.com.au/html_ver/
Open debugger via ctrl+shift+s
Select the html_ver source
Set it on line 142 it slides down to line 454 (var t_y;)

Note: I've also seen it slide to line 384 or *not slide at all*.
I confirm the same. It is not reliably reproducible which is annoying.
Flags: needinfo?(hub)
I cannot reproduce the issue where the breakpoint slides down to another location. Instead, the breakpoint stays at line 142, as expected. However, in my case, the breakpoint never gets hit.

I did some digging, and apparently the script store claims that there are no scripts that match the source actor for gallery.php and line 148. Consequently, the breakpoint actor is never set as a handler on any script.

The weird thing is that according to my log data the script in question was clearly added to the script store before we tried to set the breakpoint, so why would the script store report it as not being there?

After some more digging, I noticed that TabSources.prototype.reset gets called *after* the script on which we are trying to set a breakpoint is added to the script store, but *before* onNewScript has been called for the remaining scripts. As a result, the actorID of the source actor for the script we are trying to set a breakpoint on will be reused for the scripts that have yet to be added. When we finally set the breakpoint, the source actor ID we use to set the breakpoint no longer refers to the source we think it does.

It seems to me that resetting the TabSources while we're in the middle of loading the scripts for a page is a really bad thing. We should either do it before or after all scripts have been loaded. The rest itself happens from webbrowser.js:1515, as a result of a call to onWindowCreated (presumably by the progress listener). This shouldn't be happening as far as I can tell.

James, do you have any idea what could be going on here?
> As a result, the actorID of the source actor for the script we are trying to set a breakpoint on will be reused for the scripts that have yet to be added. When we finally set the breakpoint, the source actor ID we use to set the breakpoint no longer refers to the source we think it does.

I don't understand this part. The store that keeps a unique ID for each URL persists across an entire debugging session, as long as devtools are open. And if the ScriptStore is being blown away, it will be recreated again later and automatically add every existing script.

The ThreadStore is ephemeral, you really should be able to blow it away at any time and the only thing you lose is the cache.
I wrote "ThreadStore" but I actually meant "TabSources"
Using bgrins' STR I can get the breakpoint to slide from line 142 to line 384. The fact that it doesn't happen consistently is annoying. I'll take another look at the sliding issue tomorrow.
(In reply to Brian Grinstead [:bgrins] from comment #13)
> Created attachment 8598662 [details]
> breakpoint-sliding.gif
> 
> Here's what I see:
> 
> Open this in a new tab: http://www.serenahphotography.com.au/html_ver/
> Open debugger via ctrl+shift+s
> Select the html_ver source
> Set it on line 142 it slides down to line 454 (var t_y;)
> 
> Note: I've also seen it slide to line 384 or *not slide at all*.

The fact that it slides to line 384 at some times and line 454 at other times is caused by the fact that line 384 is in a top-level script, and line 454 in a function.

The breakpoint sliding algorithm works in such a way that it always slides to the nearest line for which we have any offsets. If the top-level script happens to be garbage collected, that is line 454. Otherwise it's line 384.

Admittedly, the fact that this behavior is gc sensitive is confusing, but I haven't found a way to do this that is less confusing. We used to slide only on the innermost script, but that won't work if there is a script with the same static level above the one we are sliding in (in that case, breakpoint sliding will fail, even when there is a line with offsets right below the one we are setting the breakpoint on).

So far, I have no explanation for the fact:
1. The breakpoint slides at all (we're setting the breakpoint on a valid line)
2. If the breakpoint doesn't slide, why it doesn't hit
I have a hunch what might be going on here. html_ver contains three script tags. Each of those scripts has its own unique Debugger.Source instance. However, each of those Debugger.Source instances has the same url. Because of that, we end up reusing the same source actor for each of them.

When we try to set a breakpoint in either of those script tags, we always end up using the source actor referring to the Debugger.Source for the last script tag, which is why (I think) the breakpoint always ends up sliding to a line within that script tag. This happens even when you try to set a breakpoint on a line that is not within a HTML comment.

The real problem here is that I think each of those sources should have been identified as inline sources. For inline sources, we convert the source actor to a sourcemapped state, so that it doesn't have a Debugger.Source instance, but it does have an originalURL. When setting a breakpoint with a sourcemapped actor, we use the url instead of the Debugger.Source instance to find scripts. All three scripts have different Debugger.Source instances, but the same url, which would have lead to the correct behavior.

Currently, we detect whether sources are inline by checking the extension of their URL. That doesn't work in this case, since the HTML file is called "html_ver/". We need a better way to detect whether sources are inline or not.

James, can you think of a quick hack here? Or do we need API support in order to solve this properly?
Flags: needinfo?(jlong)
Nevermind. Panos just pointed out to me that we can use Debugger.Source.prototype.element now to detect whether a source is inline or not. The resulting code is both simpler and seems to have solved the bug :-)
Flags: needinfo?(jlong)
Attachment #8601920 - Flags: review?(jlong)
(In reply to Eddy Bruel [:ejpbruel] from comment #21)
> Created attachment 8601920 [details] [diff] [review]
> Use Debugger.Source.prototype.element to detect whether a source is inline
> 
> Nevermind. Panos just pointed out to me that we can use
> Debugger.Source.prototype.element now to detect whether a source is inline
> or not. The resulting code is both simpler and seems to have solved the bug
> :-)

No you can't, unfortunately. See https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Source#Accessor_Properties_of_the_Debugger.Source_Prototype_Object. The `element` is "..., or source document referenced by its src attribute."

External scripts loaded with <script src="..."></script> will have an `element` property pointing to that element.

I'll think about this a little more after I have coffee.
Comment on attachment 8601920 [details] [diff] [review]
Use Debugger.Source.prototype.element to detect whether a source is inline

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

::: toolkit/devtools/server/actors/utils/TabSources.js
@@ +279,5 @@
>      // XXX bug 915433: We can't rely on Debugger.Source.prototype.text
> +    // if the source is an HTML-embedded <script> tag.
> +    if (!!aSource.element) {
> +      spec.isInlineSource = true;
> +    } else {

I'm pretty sure this is bad, every external source created from a script element has this property, even if it was loaded with the `src` attribute.

We also realy on the contentType being "text/javascript" when the source is created, otherwise when getting the contents of the source it will actually fetch the contents instead of using Debugger.Source.prototype.text.
Attachment #8601920 - Flags: review?(jlong) → review-
I wanted to do something like this very badly when I was refactoring things for Debugger.Source, but unfortunately couldn't find a way to quickly detect if something was inline or not. I *hope* I'm wrong, but I think I'm remembering correctly.
Assignee: nobody → ejpbruel
Here's a new attempt. It doesn't change your original check, it just extends the set of cases where we detect an inline source.
Attachment #8601920 - Attachment is obsolete: true
Attachment #8602605 - Flags: review?(jlong)
I had posted this comment before I left yesterday but didn't realized it got caught in a conflict :(

"Eddy brought up an idea in IRC which sounds promising: if an `element` property exists, we can check to see if it has a `src` property or not. If it doesn't, it should be an inline source.

We still need to check the URL and set the contentType to "text/javascript" because we look for that in the `_getSourceText` function. We use the raw text from Debugger.Source if it is. You may be able to remove that and only use that if there is no `_originalUrl`, otherwise fetch the original URL. I'm not sure, I think there are tests that specify why that works the way it currently does.

Thanks for solving this Eddy, I think your idea of checking the element should work great."

Nice job figuring out that solution.
Comment on attachment 8602605 [details] [diff] [review]
Use Debugger.Source.prototype.element to detect whether a source is inline

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

Looks great, with one nit. This code is pretty ugly and I'd like to improve how we fetch the source contents which would remove the need for all this contentType checking. But we'll do that later.

::: toolkit/devtools/server/actors/utils/TabSources.js
@@ +291,5 @@
> +    } else {
> +      if (url) {
> +        try {
> +          let urlInfo = Services.io.newURI(url, null, null).QueryInterface(Ci.nsIURL);
> +          if (urlInfo.fileExtension === "html" || urlInfo.fileExtension === "xml") {

Remove this if branch now that you do the inline check above
Attachment #8602605 - Flags: review?(jlong) → review+
Note that bug 1118332 specifically uses the `isInlineSource` check to figure out if it should load from the browser cache or not (the end goal is to only load the HTML file from cache, the others shouldn't be).

But the `isInlineCache` is currently faulty, and this bug fixes that, so we need to make sure to land this for devedition-40.
Try push for this patch. devedition-40 is due this monday, so let's hope we can land this today:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec186480fc97
https://hg.mozilla.org/mozilla-central/rev/e471fd92e2bb
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: