Closed Bug 341764 Opened 14 years ago Closed 13 years ago

Should be able to debug from the slow script dialog if a debugger is present

Categories

(Core :: DOM: Core & HTML, enhancement)

1.8 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

(Keywords: fixed1.8.1, late-l10n)

Attachments

(1 file, 2 obsolete files)

[Note 1: For now this seems to be better off in DOM Core, along with other slow script dialog bugs. If someone disagrees, please provide some reasoning here and feel free to move it ].

[Note 2: bug 78089 and bug 262378 (triage: is the latter a dupe of the former?)]

If I'm a webdeveloper, and I'm scripting in a webpage and get this dialog shoved in front of me, instead of clicking continue all the time, I could imagine that I might be curious *what* is taking such a long time (especially if I didn't think I'd be confronted with said dialog). So ideally, iff there is a js debugger present (q1: (how) can we check for that?), we offer a "Debug" button in addition to what we currently do. That'd probably force us to change the wording on the dialog anyway, so it'd make sense, and allow for fixing aforementioned wordage bugs along with it.

q2: Do we only want to do this for website code, or also for chrome? Can we (easily) check which of the two we're being busy in?
Attached patch Very rough patch (obsolete) — Splinter Review
This is what I currently have, and this actually works quite nicely when used in a stupid while (true) { //foo } page. However, a few problems I still have:

* If venkman is not open, the debug script button gets shown anyway (this is an optimized non-debug build w/ symbols, for what it's worth) and then does the same thing as the Continue button, apparently because of
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/jsd/jsd_hook.c&rev=3.10&mark=103-104#83
(I'm fairly sure that's the cause, but debugging a non-debug build has its weirdnesses). I think it would be preferable if the button either didn't get shown or started venkman anyhow. I think the latter is really hard: we'd somehow need to start venkman while our js is waiting for us to return control to it, so that'd require (if I recall some things Silver said last night correctly) spinning up an extra event loop to get more JS running, blah blah blah, complicated and probably not feasible in the few days we have left on branch. So I'm mainly looking not to show the button if it actually doesn't do anything. Would anyone have a clue how to improve the debugPossible PRBool's assignment statement so that actually happens?
* How do I test for the more extraordinary return values of the debug handler? Rob / Mike, could either of you give a hand in this respect? I think Rob told me yesterday that the RETURN and THROW values are used in some cases to return or throw a result of a script, but I can't work out where that happens (or what exactly Rob meant, maybe I'm misunderstanding here).
* Suppose I break into a script in this way, and then use Venkman's commandline to change to a different page (document.location = "http://www.google.com"), then hit Continue. First of all the google page will complain about no JS (expected, I think), second, the Continue statement seems to continue running the slow script, which in my case (because it was a real infinite loop) meant that I got the slow script dialog again. The latter seemed evil. Suggestions as to whether I should worry about that (and how to 'fix' that problem) are welcome!
The problem with the check is that (for all non-broken configurations) JSD is the debuggerHandler for SpiderMonkey. It would probably be possible to do something like this:

jsdService->GetDebuggerHook(&jsdHook);
PRBool debugPossible = (cx->runtime && cx->runtime->debuggerHandler) &&
    ((cx->runtime->debuggerHandler != jsd_DebuggerHandler) ||
     (jsdHook != nsnull))

(with the appropriate getting of the JSD service, if possible, etc.)

I don't know if there's a better way to interrogate the handler.
(In reply to comment #2)
> It would probably be possible to do
> something like this:
[snip]

Except that jsdI* are in the jsdebug module, which is tier_50, and we're in dom, ergo tier_9. Which means we have no way to figure this out *unless* jsd moves to tier_9. CC-ing Benjamin who has more of a clue of how feasible that is.

I'm currently building with jsd in tier_9, as one of the first items (before dom, layout and whatever) but my laptop is slow, it's still busy in layout and probably needs another hour or even more to complete. I'm tired (it's 1.40am) and I'll finish the build tomorrow. 

JSD seems to have built fine though. tier_9 also seems like a much more sane place to be than tier_50, but then again I'm not a build engineer so maybe there are reasons for it that I have no clue about. So I'm wondering whether it would be possible at all to move jsd to tier_9. I have no problem providing a patch if necessary, but right now I thought it'd probably be best to get some feedback first.

If someone has some other way to check whether JSD has a client registered (that will actually do something when JSD gets told we want it to debug something), without touching JSD itself or anything else currently in higher tiers, then please let me know. Until someone does that, I'm going to assume that'd be impossible (seems sensible - needing JSD to figure out what's using JSD).

While we're at it, *really* taking the bug.
Status: NEW → ASSIGNED
Moving it to tier-9 sounds fine to me. But you have to make sure that the --disable-jsd flag still works (minimo, for instance)
This should do *for the 1.8 branch*. I'm asking for approval for said branch. I'll attach a trunk patch shortly that can be used for baking if so desired, but please don't suggest doing so unless the purpose is to actually land this on branch if there are no significant regressions. Trunk deserves a better approach without adding an optional dependency on JSD to DOM (timeless suggested using observer... stuff, I forget what exactly). I/we do not have time to implement such a (more involved) solution on branch, but I (and I hope I can say 'we' here) do want this feature on branch so that web/extension-developers can use it.

This patch adds 2 locale strings. It doesn't change interfaces. It doesn't do anything if there is no debugger present. It does, however, move jsd to a different tier and adds a weak/optional/not-scary (wording courtesy of biesi) dependency on jsd to DOM. DOM will work without jsd, but if jsd is built it'll be used for this purpose.

I've tested it with Venkman and Firebug 0.4. Both work. Nothing seems to break even if I return out of the ordinary result codes (THROW, ERROR, RETURN_WITH_VALUE) by manipulating the internal venkman stuff while debugging.

I'm requesting review from bsmedberg for the makefile changes, from sicking for the dom changes and sr from shaver so we get a double-check on the JSD stuff. Approval request is for dom code, so I'm setting that to sicking as well. Please change this as appropriate if you do not have the time to get to this soon, or think someone else is more appropriate to do the review.

If you have any questions about what I'm doing, please let me know and I'll try to get back at you ASAP.
Attachment #225983 - Attachment is obsolete: true
Attachment #226091 - Flags: superreview?(shaver)
Attachment #226091 - Flags: review?(benjamin)
Attachment #226091 - Flags: approval-branch-1.8.1?
Attachment #226091 - Flags: review?(bugmail)
Attachment #226091 - Flags: approval-branch-1.8.1?(bugmail)
Attachment #226091 - Flags: approval-branch-1.8.1?
Attached patch Real patch [checked in on TRUNK] (obsolete) — Splinter Review
Trunk patch. Nearly identical to branch. Would have been identical if it wasn't for the existing differences in the file.
Attachment #226091 - Flags: approval-branch-1.8.1?(bugmail)
Comment on attachment 226091 [details] [diff] [review]
Real patch [1_8_BRANCH]

Looks pretty straightforward.  Wonder if we'd rather have an nsIScriptDebugger interface with methods like debugError(nsIErrorThingy) for calling from the console, and debugScript(nsICurrentScriptState) for the slow-script case.

For 1.8.1, let's do this.  We can retrofit late if we have to, IMO, and we'll probably learn more about what that interface should do as you progress here.

sr=shaver
Attachment #226091 - Flags: superreview?(shaver) → superreview+
Comment on attachment 226096 [details] [diff] [review]
Real patch [checked in on TRUNK]

We should land this on the trunk, to keep the code in sync and to make it easier to test future changes.  It won't make it any harder to DTRT on the trunk later, IMO.
Attachment #226096 - Flags: superreview+
Comment on attachment 226091 [details] [diff] [review]
Real patch [1_8_BRANCH]

>+KillScriptWithDebugMessage=A script on this page may be busy, or it may have stopped responding. You can stop the script now, debug it, or you can continue to see if the script will complete.

Nit: "You can stop the script now, open the script in the debugger, or let the script continue."
Attachment #226091 - Flags: review?(benjamin) → review+
Comment on attachment 226091 [details] [diff] [review]
Real patch [1_8_BRANCH]

>Index: dom/src/base/nsJSEnvironment.cpp
...
>+    // Check if there's a user for the debugger service that's 'on' for us
>+    if (NS_SUCCEEDED(rv))
>+    {

Nit: please follow file convention and put the { on the same line as the |if|.

>+    // If there is a debug handler registered for this runtime AND
>+    // ((jsd is on AND has a hook) OR (jsd isn't on (something else debugs)))
>+    // then something useful will be done with our request to debug.
>+    debugPossible = ((jsds_IsOn && (jsdHook != nsnull)) || !jsds_IsOn);

This can be simplifed to:
  !jsds_IsOn || jsdHook != nsnull

or even
  !jsds_IsOn || jsdHook

But i'm ok if you want to keep the current expression for clarity.

r=sicking with at least the first comment fixed
Attachment #226091 - Flags: review?(bugmail) → review+
Comment on attachment 226091 [details] [diff] [review]
Real patch [1_8_BRANCH]

(In reply to comment #10)
> Nit: please follow file convention and put the { on the same line as the |if|.
Oops, fixed. Here and for the switch {...} later on.

> But i'm ok if you want to keep the current expression for clarity.
That seemed best to me - if more people object I'm willing to change it ;-).

I'm requesting branch approval. Patch touches makefiles and was therefor also reviewed by build people (thanks bsmedberg!). It shouldn't affect anyone not building jsd, and should not affect 99% of our userbase (who never use a debugger). I tested it in just about any way I could think of, some of the tests on Linux, some on Windows, and I didn't hit any problems.
Attachment #226091 - Flags: approval1.8.1?
Checked in on trunk, would still like this to go on branch, obviously :-)

Leaving bug open for future discussion of a better fix for trunk, if we need it. I won't get to that before this lands on branch or is denied approval, though :-).
Attachment #226096 - Attachment description: Real patch [TRUNK] → Real patch [checked in on TRUNK]
Attachment #226096 - Attachment is obsolete: true
Comment on attachment 226091 [details] [diff] [review]
Real patch [1_8_BRANCH]

a=darin on behalf of drivers (please land this on the MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword to this bug)
Attachment #226091 - Flags: approval1.8.1? → approval1.8.1+
Landed on branch.
Keywords: fixed1.8.1
we should file another bug for any additional work on the trunk. that way testers and localizers will see this bug is fixed on the branch and do the right thing for  verification and localizations...
Blocks: 346152
(In reply to comment #15)
> we should file another bug for any additional work on the trunk. that way
> testers and localizers will see this bug is fixed on the branch and do the
> right thing for  verification and localizations...
Filed, marked dep, marking this FIXED.
No longer blocks: 346152
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Blocks: 346152
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.