Closed Bug 897567 Opened 11 years ago Closed 10 years ago

JS debugger: setting breakpoints can be confused by script GC

Categories

(DevTools :: Debugger, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: jimb, Assigned: jjurovych, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(2 files, 5 obsolete files)

I think thread actors may return error replies to setBreakpoint packets when they should not, depending on GC timing.

In toolkit/devtools/server/actors/script.js, the function _setBreakpoint calls dbg.findScripts to see if any scripts match the location given in the setBreakpoint request, and returns an error reply if none exist.

Debugger.prototype.findScripts operates by sweeping debuggee compartments looking for scripts that match the query. This means that findScripts can sometimes return scripts that are no longer referenced, but have not yet been recognized as garbage by the garbage collector. Whether findScripts returns such scripts is non-deterministic, as it depends on the relative timing of the debugger and debuggee's activities versus those of the collector.

This is unfortunate, but hard to solve without changes to the Debugger API; short of traversing the object graph itself, findScripts has no way to avoid the problem. (Now that we have Debugger.Source, it might be possible to redesign things to avoid it...) But the non-determinism is documented; caveat implementor, right? :(

I may be misunderstanding script.js, but it seems to me we can return an error when we should not, under the following circumstances:

1) The developer visits the page.
2) In response to user activity, the page loads foo.js.
3) The developer reloads the page. This refresh has not yet loaded foo.js.
4) The developer tries to set a breakpoint on foo.js.

At that point, whether or not the this.dbg.findScripts call in ThreadActor.prototype._setBreakpoint returns any scripts depends on whether foo.js's script was GC'd. If it has been, then the client will be unable to set the breakpoint at all.

Fix: Ignore the protocol spec's discussion of "noScript" errors, and always reply to setBreakpoint packets with success. If no scripts are found, leave the breakpoint in the store to be applied to scripts loaded in the future.
Similarly, the call to findScripts in onSetBreakpoint should probably go. The sanity checks for 'line' seem like they should come at the top of the function, before we bother passing it to getGeneratedLocation; and the findScripts check should not happen at all.
Priority: -- → P3
(In reply to Jim Blandy :jimb from comment #0)
> Fix: Ignore the protocol spec's discussion of "noScript" errors, and always
> reply to setBreakpoint packets with success. If no scripts are found, leave
> the breakpoint in the store to be applied to scripts loaded in the future.

Yep, we should do this.

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#1339
http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#1426
Whiteboard: [mentor=nfitzgerald@mozilla.com][lang=js]
I think this would also solve bug 979571, although we should still figure out what broke it.
Mentor: nfitzgerald
Whiteboard: [mentor=nfitzgerald@mozilla.com][lang=js] → [lang=js]
Assignee: nobody → jjurovych
Status: NEW → ASSIGNED
Attached patch bug-897567.patch (obsolete) — Splinter Review
Instead of throwing an error, this patch treats non-existing URLs as valid.
Comment on attachment 8449834 [details] [diff] [review]
bug-897567.patch

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

Looks good! Make the small, requested changes below and then reattach the new patch. I'll do a try push[0] and if it comes back green, we can land it :)

[0] https://wiki.mozilla.org/ReleaseEngineering/TryServer

::: toolkit/devtools/server/actors/script.js
@@ +1512,1 @@
>          actor: actor.actorID

Because it isn't obvious why we return "successfully" here, we should probably have a comment above the return describing the logic for future readers. Something like:

  Since we did not find any scripts to set the breakpoint on now, return
  early. When a new script that matches this breakpoint location is
  introduced, the breakpoint actor will already be in the breakpoint store
  and will be set at that time.

::: toolkit/devtools/server/tests/unit/test_breakpoint-19.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/**
> + * Check that we only break on offsets that are entry points for the line we are
> + * breaking on. Bug 907278.

Please change this test summary so it describes what it is actually doing.
Attachment #8449834 - Flags: review+
@fitzgen: Done. Should I fold these patches into one?
Attachment #8449898 - Flags: review+
(In reply to Jakub Jurovych from comment #6)
> Created attachment 8449898 [details] [diff] [review]
> Address https://bugzilla.mozilla.org/show_bug.cgi?id=897567#c5
> 
> @fitzgen: Done. Should I fold these patches into one?

Yes, please. Generally attach a patch for each thing that should be a commit on m-c. When we have larger patches, we tend to split them up to make it easier to grok/review, like "Part 1 - RDP changes in server and client to support XYZ" and "Part 2 - Make debugger frontend use XYZ". We try not to make any patches/commits that won't function without the next part. This helps bisecting tools a lot.
Attached patch bug-897567-2.patch (obsolete) — Splinter Review
Attachment #8449834 - Attachment is obsolete: true
Attachment #8449898 - Attachment is obsolete: true
Attachment #8450359 - Flags: review+
Attachment #8450359 - Flags: review+ → review?(nfitzgerald)
Comment on attachment 8450359 [details] [diff] [review]
bug-897567-2.patch

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

It looks like you just concatenated the two separate patch files, when we need to merge/fold them together into one patch file. I can't find you on IRC, but if you want to come up to my desk I can help you fix this.
Attachment #8450359 - Flags: review?(nfitzgerald)
Attached patch bug-897567.patch (obsolete) — Splinter Review
This patch should be folded properly, sorry about that.
Attachment #8450359 - Attachment is obsolete: true
Jakub, now that you have commit level 1, can you do a try push[0] for this patch and if it comes back green, add the "checkin-needed" keyword?

[0] https://wiki.mozilla.org/ReleaseEngineering/TryServer

This try syntax should be good:

  try: -b do -p all -u xpcshell,mochitest-dt -t none
Attached patch bug-897567-2.patch (obsolete) — Splinter Review
Looks like this one passes on Try. One older test had to be rewritten.
Attachment #8451799 - Attachment is obsolete: true
Attachment #8458239 - Flags: checkin?(nfitzgerald)
Comment on attachment 8458239 [details] [diff] [review]
bug-897567-2.patch

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

Sorry just found a bug. Can you update and flag me for review one last time?

::: toolkit/devtools/server/actors/script.js
@@ +135,3 @@
>      }
>  
>      this._size++;

Ah, I think I just found a bug:

  bps = new BreakpointStore();
  loc = { url: "a.js", line: 1 };
  bps.addBreakpoint(loc);
  bps.addBreakpoint(loc);

  // false, but should be true since we reuse the existing BP
  bps.size == 1

Can you modify toolkit/devtools/server/tests/unit/test_breakpointstore.js to exercise this case?

Also, can you modify |addBreakpoint| to return the breakpoint, so that if we reuse an existing BP, the caller gets a reference to it. Will need to update doc comment, and any callers that addBreakpoint(location) and then continue to mutate location.
Attachment #8458239 - Flags: checkin?(nfitzgerald)
Oh and FYI we use the "checkin-needed" keyword rather than setting the "checkin?" flag because then any of the sheriffs will land the patch for you (along with a bunch of other patches, which saves TBPL/try server resources) instead of waiting for some particular person to checkin your patch.
Attachment #8458239 - Attachment is obsolete: true
(In reply to Nick Fitzgerald [:fitzgen] from comment #13)
> Can you modify toolkit/devtools/server/tests/unit/test_breakpointstore.js to
> exercise this case?
Done.

> Also, can you modify |addBreakpoint| to return the breakpoint, so that if we
> reuse an existing BP, the caller gets a reference to it. Will need to update
> doc comment, and any callers that addBreakpoint(location) and then continue
> to mutate location.
I haven't found anything that depends on this, but I changed it anyway since it makes more sense.
Attachment #8458986 - Flags: review?(nfitzgerald)
Comment on attachment 8458986 [details] [diff] [review]
bug-897567-2.patch

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

Looks good! r+ with change below, unless I'm mistaken about that.

No need to ask for review again, just make the small change, re-attach, and then add the "checkin-needed" keyword.

Thanks :)

::: toolkit/devtools/client/dbg-client.jsm
@@ +1761,5 @@
>              location,
>              root.traits.conditionalBreakpoints ? condition : undefined
>            );
> +        }
> +        if (aResponse) {

Should be "if (aOnResponse) {" right?
Attachment #8458986 - Flags: review?(nfitzgerald) → review+
(In reply to Nick Fitzgerald [:fitzgen] from comment #18)
> Should be "if (aOnResponse) {" right?
Right, sorry.

Try: https://tbpl.mozilla.org/?tree=Try&rev=f6029bbd7de3
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/2f82f5227f21
Keywords: checkin-needed
Whiteboard: [lang=js] → [lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2f82f5227f21
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [lang=js][fixed-in-fx-team] → [lang=js]
Target Milestone: --- → Firefox 34
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.