Closed Bug 1302460 Opened 3 years ago Closed 3 years ago

Debugger breakpoints are broken in the browser toolbox

Categories

(DevTools :: Debugger, defect)

50 Branch
defect
Not set

Tracking

(firefox49 unaffected, firefox50 fixed, firefox51 fixed, firefox52 fixed)

RESOLVED FIXED
Tracking Status
firefox49 --- unaffected
firefox50 --- fixed
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: Gijs, Assigned: ejpbruel)

References

Details

(Keywords: regression)

Attachments

(1 file)

STR:

1. open browser toolbox
2. search for browser.js
3. open chrome://browser/content/browser.js (the main/huge browser.js file)
4. in the file, search for browserGoHome function
5. set breakpoint on the first line
6. in the main browser window from which you started the toolbox, click the 'home' button.

ER:
break

AR:
no break

Additional info: if you modify browser.js and insert a debugger; statement, the browser debugger does stop if you hit the home button. If you then add a breakpoint on the next 'real' line in of code and hit 'continue', it still does not stop on that breakpoint.
I've seen this before but intermittently and depending on the profile but I can reproduce this consistently even on a clean profile.

Tried with new debugger frontend and it has the same problem.
If I do 'pause on next execution' (F8) it does pause as expected, so it's something to do with breakpoints
If I set a breakpoint in toolbox.js on the first line in "function Toolbox" then open a devtools toolbox it does break, so appears to be dependent on the script
Debugging this is clumsy, but possible.  If you add logging in the actor code it will show up in the Browser Console for the browser being debugged.  So, I put a log in the 'hit' function: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/breakpoint.js#136 and it does fire in the toolbox.js case but not in the browser.js
In the case of browser.js, the breakpoint actor is initialized (https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/breakpoint.js#46) but addScript is never called (https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/breakpoint.js#72).

BreakpointActor initialize:
  * ThreadActor: Object { _state: "paused", _frameActors: Array[0], _parent: Object, _dbg: Debugger, _threadLifetimePool: Object, _tabClosed: false, _scripts: null, _pauseOnDOMEvents: null, _options: Object, breakpointActorMap: Object, 26 more… }
  * OriginalLocation: Object { _connection: Object, _actorID: "server2.conn0.486", _line: 1794, _column: undefined, _name: undefined }

In the case of toolbox.js both functions are called: 
BreakpointActor initialize:
  * ThreadActor: Object { _state: "paused", _frameActors: Array[0], _parent: Object, _dbg: Debugger, _threadLifetimePool: Object, _tabClosed: false, _scripts: null, _pauseOnDOMEvents: null, _options: Object, breakpointActorMap: Object, 26 more… } 
  * OriginalLocation: Object { _connection: Object, _actorID: "server2.conn0.562", _line: 110, _column: undefined, _name: undefined }  breakpoint.js:49

BreakpointActor addScript:
  * Script { displayName: "Toolbox", url: "resource://gre/modules/commonjs/too…", startLine: 109, lineCount: 67, source: Source, sourceStart: 4629, sourceLength: 2792, global: Object, format: "js" }

Eddy, do you have any idea why addScript wouldn't be getting called when setting a breakpoint in Browser Toolbox on browser.js?  Can you help debug what's going on here (see Comment 4)?
Flags: needinfo?(ejpbruel)
(In reply to Brian Grinstead [:bgrins] from comment #5)
> In the case of browser.js, the breakpoint actor is initialized
> (https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/
> breakpoint.js#46) but addScript is never called
> (https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/
> breakpoint.js#72).
> 
> BreakpointActor initialize:
>   * ThreadActor: Object { _state: "paused", _frameActors: Array[0], _parent:
> Object, _dbg: Debugger, _threadLifetimePool: Object, _tabClosed: false,
> _scripts: null, _pauseOnDOMEvents: null, _options: Object,
> breakpointActorMap: Object, 26 more… }
>   * OriginalLocation: Object { _connection: Object, _actorID:
> "server2.conn0.486", _line: 1794, _column: undefined, _name: undefined }
> 
> In the case of toolbox.js both functions are called: 
> BreakpointActor initialize:
>   * ThreadActor: Object { _state: "paused", _frameActors: Array[0], _parent:
> Object, _dbg: Debugger, _threadLifetimePool: Object, _tabClosed: false,
> _scripts: null, _pauseOnDOMEvents: null, _options: Object,
> breakpointActorMap: Object, 26 more… } 
>   * OriginalLocation: Object { _connection: Object, _actorID:
> "server2.conn0.562", _line: 110, _column: undefined, _name: undefined } 
> breakpoint.js:49
> 
> BreakpointActor addScript:
>   * Script { displayName: "Toolbox", url:
> "resource://gre/modules/commonjs/too…", startLine: 109, lineCount: 67,
> source: Source, sourceStart: 4629, sourceLength: 2792, global: Object,
> format: "js" }
> 
> Eddy, do you have any idea why addScript wouldn't be getting called when
> setting a breakpoint in Browser Toolbox on browser.js?  Can you help debug
> what's going on here (see Comment 4)?

I'm taking a look at this now.
Assignee: nobody → ejpbruel
Flags: needinfo?(ejpbruel)
I bisected this down with mozregression to bug 1238173.

As per Eddy on IRC, this might still be a bug unrelated to the script store removal in that it seems wrong that the Script has disappeared (been GC'd) even without the script store.
(In reply to :Gijs Kruitbosch from comment #7)
> I bisected this down with mozregression to bug 1238173.
> 
> As per Eddy on IRC, this might still be a bug unrelated to the script store
> removal in that it seems wrong that the Script has disappeared (been GC'd)
> even without the script store.

Some observations that I've done in the meantime:

To set a breakpoint in BrowserGoHome, we find all Debugger.Script instances that match the Debugger.Source instance for browser.js, as well as the line number for the breakpoint. The debugger API claims the are no such Debugger.Script instances. This is why addScript never gets called, as bgrins observed.

There are two explanations for why Debugger.findScripts returns no matches:
1. The script we are looking for is garbage collected.
2. We are using the wrong Debugger.Source instance.

The first explanation is consistent with bgrins' observation that the problem was originally intermittent, and gijs' observation that the script store removal seems to have triggered it (the script store prevented scripts from being gc'd while the debugger is open).

However, gijs also observed that debugger statements *do* work in the same function, which contradicts the first explanation. It seems more likely then, that we are using the wrong Debugger.Source instance. This can happen when the same source file is compiled more than once. In that case, we get a different set of Debugger.Script instances for each compilation, each with their own Debugger.Source instance.

For content debugging, this means that we need to update the source actors to point to the new Debugger.Source instances whenever a page is reloaded (and thus recompiled). If we fail to do so, setting a breakpoint will cause us to set a breakpoint on a previous Debugger.Script instance, which either no longer exists (because it has been gc'd), or will have no effect (since it's not being executed anymore).

I am not sure what implications this has for chrome debugging. As an experiment, instead of finding all scripts that match a given Debugger.Source instance, I wrote some code that simply finds all scripts, and then print those scripts that have a Debugger.Source instance with the same URL as the one we are looking for, as well as a line number range that covers the line we are looking for.

The output looks something like this:

FINDING SCRIPTS THAT MATCH QUERY {"line":1794,"source":{}}
FOUND 0 SCRIPTS THAT MATCH QUERY
TOTAL NUMBER OF SCRIPTS 19397
	SCRIPT WITH SOURCE chrome://browser/content/browser.js START LINE 1793
	SCRIPT WITH SOURCE chrome://browser/content/browser.js START LINE 1793

It seems that there are at least two Debugger.Script instances around for BrowserGoHome. I am not sure why, but the fact that there is more than one definitely opens up the possibility that we are using a Debugger.Source instance for a Debugger.Script instance that no longer exist.
Here's another test I did:

FINDING SCRIPTS WITH CANONICAL SOURCE ID 4886343552
FOUND 0 SCRIPTS
FINDING SCRIPTS WITH URL chrome://browser/content/browser.js
FOUND 2 SCRIPTS
SCRIPT WITH CANONICAL SOURCE ID 4894767712
SCRIPT WITH CANONICAL SOURCE ID 4886343552

First I use findScripts with the Debugger.Source instance as the query. That yields 0 scripts. Then I use findScripts again, this time using the URL of the Debugger.Source instance as the query. That yields 2 scripts, one of which has the Debugger.Source instance we were originally looking for as its source (it has the same canonical ID, so it must be the same).

It seems like findScripts is lying to us. The plot thickens.
Attached patch Potential fixSplinter Review
Since ScriptSources are never moved by the GC, their address is used as the canonicalID of  Debugger.Source. If two Debugger.Sources have the same canonicalID, they should always compare equal. In comment 9, findScripts is unable to find a JSScript matching the Debugger.Source with canonicalID 4886343552. However, calling findScripts with the URL of that Debugger.Sources lists two scripts, one of which definitely has a Debugger.Source with canonicalID 4886343552.

So, what's going on? Apparently, under some circumstances that I can't quite figure out, a JSScript and a Debugger.Source can have distinct ScriptSourceObjects referring to the same ScriptSource. This is why Debugger.findScript seems to lie to us: it compares ScriptSourceObjects, not ScriptSources. If I change this code to compare ScriptSources instead (see attached patch), the problem reported in comment 1 goes way. Consequently, there cannot be a 1:1 relationship between ScriptSourceObjects and ScriptSources.

Problem is, as far as I can tell from the SpiderMonkey source code, ScriptSourceObjects and ScriptSources *do* have a 1:1 relationship; we never seem to create a ScriptSourceObject from a ScriptSource that wasn't newly created.

So here's my current situation. I have a fix that solves the problem. I have a theory why it solves the problem. But I can't prove my theory. Jim or Shu, can any of you help out here and either confirm or deny my analysis?
Flags: needinfo?(shu)
Attachment #8791229 - Flags: feedback?(jimb)
Comment on attachment 8791229 [details] [diff] [review]
Potential fix

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

I believe there are cases where two ScriptSourceObjects refer to the same ScriptSource. I think it arises when we clone code into a new compartment.

If I'm understanding correctly, this patch makes dbg.findScripts({ source: S }), where S is some Debugger.Source, return Debugger.Scripts whose .source is not S, but is instead some other Debugger.Source that refers, via a different ScriptSourceObject, to the same underlying ScriptSource. I don't think that's a good idea: if dbg.findScripts({ source: S }) returns a script, then its source should be S.

If the debuggee can have multiple Debugger.Sources for what the user would consider the same source, why aren't all the relevant Debugger.Sources being considered when we set breakpoints there? Doesn't a source actor correspond to multiple Debugger.Sources?

(It seems that the "stable IDs" I mentioned in our stand-up today are not relevant to this problem at all; I misunderstood the problem then.)
Attachment #8791229 - Flags: feedback?(jimb) → feedback-
(In reply to Jim Blandy :jimb from comment #11)
> 
> If the debuggee can have multiple Debugger.Sources for what the user would
> consider the same source, why aren't all the relevant Debugger.Sources being
> considered when we set breakpoints there? Doesn't a source actor correspond
> to multiple Debugger.Sources?

I don't think that was ever a requirement for the current server architecture. I personally never knew that multiple D.S could exist for the same source! So the current code does not handle multiple D.S for a source. A source actor corresponds to a single Debugger.Source. It's complicated enough with sourcemaps, I can't imagine how much more complicated this will make the code... But that doesn't mean we won't do it. Is there no other way to hide this complexity from the Debugger API?
(In reply to James Long (:jlongster) from comment #12)
> (In reply to Jim Blandy :jimb from comment #11)
> > 
> > If the debuggee can have multiple Debugger.Sources for what the user would
> > consider the same source, why aren't all the relevant Debugger.Sources being
> > considered when we set breakpoints there? Doesn't a source actor correspond
> > to multiple Debugger.Sources?
> 
> I don't think that was ever a requirement for the current server
> architecture. I personally never knew that multiple D.S could exist for the
> same source! So the current code does not handle multiple D.S for a source.
> A source actor corresponds to a single Debugger.Source. It's complicated
> enough with sourcemaps, I can't imagine how much more complicated this will
> make the code... But that doesn't mean we won't do it. Is there no other way
> to hide this complexity from the Debugger API?

Okay --- I thought this might be something we already dealt with that simply wasn't broken. If it's a major change, then we need to push through and get more details as to why there are multiple Debugger.Source instances for the same ScriptSource. 

Eddy, one way to solve this would be for Debugger::wrapVariantReferent (I should have asked Shu for a better name, that's the function that looks up the Debugger.Source for a given ScriptSourceObject) actually just use the ScriptSource as the key, not the ScriptSourceObject. I can't imagine that the multiple ScriptSourceObjects for a given ScriptSource differ, so it wouldn't matter if wrapping produced a Debugger.Source that referred to a different-but-equivalent ScriptSourceObject, as long as it referred to the same underlying ScriptSource.

But it'd be best to know why we're even getting the multiple ScriptSourceObjects first.
(In reply to Jim Blandy :jimb from comment #11)
> Comment on attachment 8791229 [details] [diff] [review]
> Potential fix
> 
> Review of attachment 8791229 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I believe there are cases where two ScriptSourceObjects refer to the same
> ScriptSource. I think it arises when we clone code into a new compartment.
> 
> If I'm understanding correctly, this patch makes dbg.findScripts({ source: S
> }), where S is some Debugger.Source, return Debugger.Scripts whose .source
> is not S, but is instead some other Debugger.Source that refers, via a
> different ScriptSourceObject, to the same underlying ScriptSource. I don't
> think that's a good idea: if dbg.findScripts({ source: S }) returns a
> script, then its source should be S.
> 
> If the debuggee can have multiple Debugger.Sources for what the user would
> consider the same source, why aren't all the relevant Debugger.Sources being
> considered when we set breakpoints there? Doesn't a source actor correspond
> to multiple Debugger.Sources?
> 
> (It seems that the "stable IDs" I mentioned in our stand-up today are not
> relevant to this problem at all; I misunderstood the problem then.)

I'd like to make the case that the Debugger should never expose more than one Debugger.Source instance for what effectively amounts to the same source. There is no practical benefit to do so, and it leads to subtle problems such as the one in this bug.

As a matter of fact, exposing multiple Debugger.Source instances for the same source conflicts with the semantics of Debugger.Source.prototype.canonicalId. A canonicalId is supposed to be a unique identifier that allows us to distinguish Debugger.Source instances from each other: if (and only if) two Debugger.Source instances have the same canonicalId, they are supposed to be the same.

The value of canonicalId is based on the pointer address of the ScriptSource. Since the referent of a Debugger.Source instance is a ScriptSourceObject, this presupposes a 1:1 relationship between ScriptSourceObject and ScriptSource.

Like Jim said in comment 13, I think the first thing we should do is figure out why we can have multiple ScriptSourceObjects pointing to the same ScriptSource. Till, perhaps you are able to shed some light on this?
Flags: needinfo?(till)
I'm not, sorry. AFAICT, the last person to meaningfully touch the ScriptSourceObject code is Jim in bug 1031636. Whether some later change introduced this issue or it was present since (or even before) that patch, I don't know.
Flags: needinfo?(till)
Needinfo ping @shu for comment 10.
(In reply to Eddy Bruel [:ejpbruel] from comment #10)
> Created attachment 8791229 [details] [diff] [review]
> Potential fix
> 
> Since ScriptSources are never moved by the GC, their address is used as the
> canonicalID of  Debugger.Source. If two Debugger.Sources have the same
> canonicalID, they should always compare equal. In comment 9, findScripts is
> unable to find a JSScript matching the Debugger.Source with canonicalID
> 4886343552. However, calling findScripts with the URL of that
> Debugger.Sources lists two scripts, one of which definitely has a
> Debugger.Source with canonicalID 4886343552.
> 
> So, what's going on? Apparently, under some circumstances that I can't quite
> figure out, a JSScript and a Debugger.Source can have distinct
> ScriptSourceObjects referring to the same ScriptSource. This is why
> Debugger.findScript seems to lie to us: it compares ScriptSourceObjects, not
> ScriptSources. If I change this code to compare ScriptSources instead (see
> attached patch), the problem reported in comment 1 goes way. Consequently,
> there cannot be a 1:1 relationship between ScriptSourceObjects and
> ScriptSources.
> 
> Problem is, as far as I can tell from the SpiderMonkey source code,
> ScriptSourceObjects and ScriptSources *do* have a 1:1 relationship; we never
> seem to create a ScriptSourceObject from a ScriptSource that wasn't newly
> created.
> 
> So here's my current situation. I have a fix that solves the problem. I have
> a theory why it solves the problem. But I can't prove my theory. Jim or Shu,
> can any of you help out here and either confirm or deny my analysis?

From reading the code, I think both you and jimb are mistaken about what's going on.

From reading the code, ISTM you are correct that ScriptSourceObjects exist in 1:1 relation to ScriptSources. But: two Debugger.Sources having the same canonicalId doesn't imply identity for the Debugger.Sources, though, because Debugger.Sources are basically Debugger's CCWs for the SSOs, and there is a different D.S wrapper per compartment.

To jimb's point about cross-compartment stuff. We wrap the SSO on clone instead of creating a new SSO [1]. This might mean that the SSO-SS split is useless but nobody has yet cleaned it up.

So what's going on? The reason for the behavior you see is very subtle. Can you spot the bug on this line [2]? Recall that Debugger.Source is basically a special CCW. So its SSO referent is unwrapped. script->sourceObject(), however, may be a regular CCW, and we aren't unwrapping it, so it is not guaranteed to have the same identity.

Given this, Eddy's fix is correct. But we probably should collapse the SSO-SS split because it is subtle and confusing.

[1] http://searchfox.org/mozilla-central/source/js/src/jsscript.cpp#3275
[2] http://searchfox.org/mozilla-central/source/js/src/vm/Debugger.cpp#4455
Flags: needinfo?(shu)
Another fix would be to call the poorly named JSScript::scriptSourceUnwrap.
(In reply to Shu-yu Guo [:shu] from comment #17)
> To jimb's point about cross-compartment stuff. We wrap the SSO on clone
> instead of creating a new SSO [1]. This might mean that the SSO-SS split is
> useless but nobody has yet cleaned it up.

Are you sure about this? I distinctly remember being told that ScriptSources did not belong to any compartment, and could not be made to do so, because they were shared by JSScripts in multiple compartments. I think this was used by self-hosted code, and by XUL. This is why they have the reference count ScriptSource::refs to track the references from multiple compartments.

Originally, I wanted to put all the private slots that now appear on ScriptSourceObject in ScriptSource, but this angered the GC because it created what amounted to cross-compartment references that did not go through any wrapper.

I think you are absolutely right about the failure to unwrap, though. Thanks very much for looking into this!

Permalinks For Posterity (tm):
[1] http://searchfox.org/mozilla-central/rev/dbbbafc979a8362a1c8e3e99dbea188fc832b1c5/js/src/jsscript.cpp#3275
[2] http://searchfox.org/mozilla-central/rev/dbbbafc979a8362a1c8e3e99dbea188fc832b1c5/js/src/vm/Debugger.cpp#4455
See Also: → 1302222
Just to cut down on the confusion a bit: The concerns I raised in comment 11 are based on a misunderstanding. Shu and I are pretty sure that, although distinct Debuggers will each have their own, distinct Debugger.Sources referring to a single ScriptSourceObject, for a given Debugger, there should be only one Debugger.Source per ScriptSourceObject, and there is no evidence that this isn't the case.
Depends on: 1304523
Attachment #8791229 - Flags: feedback- → review+
See Also: 1302222
Duplicate of this bug: 1302222
Is this ready to land?
Flags: needinfo?(jimb)
Flags: needinfo?(ejpbruel)
It is fine with me, but Eddy should verify.
Flags: needinfo?(jimb)
Assuming my fix is indeed correct, as Shu says, I cannot think of a reason we shouldn't land this.
Flags: needinfo?(ejpbruel)
I somehow managed to accidentally land this patch as part of bug 1288084 ¯\_(ツ)_/¯

Apparently, this didn't lead to any problems, and since the patch had r+ anyway, so there's no harm done.
Comment on attachment 8791229 [details] [diff] [review]
Potential fix

Approval Request Comment
[Feature/regressing bug #]: Unknown
[User impact if declined]: Breakpoints will not work under some circumstances in the browser debugger.
[Describe test coverage new/current, TreeHerder]: No test coverage
[Risks and why]: Low risk. The patch changes an identity test for two object from comparing the objects themselves to comparing a property on these objects that we know will be identical iff the objects are identical.
[String/UUID change made/needed]: N/A
Attachment #8791229 - Flags: approval-mozilla-beta?
Attachment #8791229 - Flags: approval-mozilla-aurora?
Resolving this so it shows up on relman's radar. This landed in https://hg.mozilla.org/mozilla-central/rev/496081cb3214 .
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Version: 51 Branch → 52 Branch
Version: 52 Branch → 50 Branch
Comment on attachment 8791229 [details] [diff] [review]
Potential fix

Fixes a recent regression in Fx50, Aurora51+, Beta50+
Attachment #8791229 - Flags: approval-mozilla-beta?
Attachment #8791229 - Flags: approval-mozilla-beta+
Attachment #8791229 - Flags: approval-mozilla-aurora?
Attachment #8791229 - Flags: approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.