Breakpoints do not show up in Source Code view

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
Document Navigation
--
critical
RESOLVED FIXED
13 years ago
12 years ago

People

(Reporter: Ian Neal, Assigned: Biesinger)

Tracking

({regression})

Trunk
mozilla1.9alpha1
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
Using BuildID 2005122808 on Windows XP
Steps to reproduce:
1. Start up SeaMonkey
2. Start JS Debugger
3. Expand a JS file in Loaded Scripts window
4. Double click so script appears in Source Code window
5. Click by side of line you want breakpoint to be at

Expected results:
1. Red B at side and line highlighted
2. Breakpoint listed in breakpoints window

Actual results:
1. Breakpoint only listed in breakpoints no red B in source code window.
I've noticed this as well as the "current line" highlighting no longer working when stepping through code. I doubt it's a venkman specific issue since I've been using the same venkman for a while now on the trunk and have only recently seen the problem.

Comment 2

13 years ago
Seeing this as well. I'm running trunk version of venkman on OS X, and have tried different themes.
(Reporter)

Comment 3

13 years ago
Regression window is between buildIDs 2005121708 and 2005121809 (also for bug 321930)

Comment 4

13 years ago
Probably Bug 309525 broke this.

Comment 5

13 years ago
What's the correlation to that bug?
I was thinking bug 289517 was a likelier candidate.

Updated

13 years ago
Severity: major → critical

Comment 7

13 years ago
Ok this was "caused" by Bug 309525 (tested by a lokal backout). The problem is the channel impl in Venkman is not correct:
259 function JSDChannel (uri)
260 {
261     this.URI = uri;
262     this.originalURI = uri;
263     this._isPending = true;

isPending should not/is not allowed to return true until AsyncOpen was called (according to biesi). But maybe this will be changed again (he said he'll discuss this with darin again on irc).
Yeah, so.. nsIRequest seems to allow this implementation of isPending. Was that the intent? If so, I should change my uriloader code...
Assignee: rginda → cbiesinger
There's no asyncOpen for requests.... :(

Perhaps nsIChannel could document the extra restriction for requests that happen to be channels?
well, it could phrase it like "isPending must be true from the time the request started until it completes, and false otherwise" or something. 
Ah, true.  I think that would make sense in general...

Comment 12

12 years ago
All these regressions to venkman makes it totally unusable... and it's blocking useful work for lots of people.  

Since the cause is known, can we just get some fix checked in please?
Håkan, feel free to contribute a patch!  The problem is that venkman's impl of nsIChannel is buggy, basically.  Want to fix it?

Comment 14

12 years ago
bisei made a patch for testing - was that not the full fix?

If it wasn't sufficient, I can try to take some time to take a look at this.
There are no patches attached to this bug.  So I have no idea what you're talking about.

Comment 16

12 years ago
Yeah, that was tested outside this bug I think :) So the question is rather for biesi if that is sufficient or if more work would be needed.
well... for venkman, it's sufficient. what I really want to do, though, is fix uriloader to handle that case.
Is this the same issue as bug 321930? Can it be duped or marked as being blocked by this?

Comment 19

12 years ago
nsIRequest::isPending is meant to be true when there is an outstanding asynchronous event waiting to happen that will make the request no longer be pending.  The point of the attribute is to flag this condition.  A request shouldn't start out with isPending true IMO.  It should only become true once there is some pending asynchronous event that will make it not be pending ;-)

We should amend the nsIRequest documentation to make this clear.
Created attachment 209232 [details] [diff] [review]
part I: Fix venkman (checked in)

ok, thanks. Here, then, is part 1 of the patch, which fixes the venkman channel impl.

I'll attach a part 2 later, which will make uriloader handle this better, and which will change the docs in nsIRequest (a frozen iface :-/)
Attachment #209232 - Flags: superreview?(darin)
Attachment #209232 - Flags: review?
Attachment #209232 - Flags: review? → review?(silver)

Comment 21

12 years ago
Comment on attachment 209232 [details] [diff] [review]
part I: Fix venkman (checked in)

r=silver on the condition that having isPending false until asyncOpen is called does not break in Mozilla 1.0, 1.4, 1.7 or 1.8 (as I understand it, you've just made something more strict, so it is probably ok).
Attachment #209232 - Flags: review?(silver) → review+
yeah, that should be fine.

Comment 23

12 years ago
Comment on attachment 209232 [details] [diff] [review]
part I: Fix venkman (checked in)

Perhaps it would be good to only set this._isPending to true when we know for sure that jsdch_aopen is going to succeed.  Same goes for the other variables perhaps?

sr=darin
Attachment #209232 - Flags: superreview?(darin) → superreview+
Created attachment 209518 [details] [diff] [review]
part II: Make uriloader handle this better
Attachment #209518 - Flags: superreview?(darin)
Attachment #209518 - Flags: review?(bzbarsky)
Created attachment 209528 [details] [diff] [review]
part III: Improve nsIRequest docs

this attempts to make the nsIRequest docs a bit clearer
Attachment #209528 - Flags: superreview?(darin)
Attachment #209528 - Flags: review?(darin)

Comment 26

12 years ago
Comment on attachment 209528 [details] [diff] [review]
part III: Improve nsIRequest docs

This is a good improvement.  I would also add something along the lines of what I said above:

"nsIRequest::isPending is ... true when there is an outstanding asynchronous event ... that will make the request no longer be pending."
Attachment #209528 - Flags: superreview?(darin)
Attachment #209528 - Flags: superreview+
Attachment #209528 - Flags: review?(darin)
Attachment #209528 - Flags: review+
Comment on attachment 209518 [details] [diff] [review]
part II: Make uriloader handle this better

>Index: nsURILoader.cpp

Did you mean to call the new method from OpenURI and pass PR_FALSE?

If so, r=bzbarsky if you actually do that.
Attachment #209518 - Flags: review?(bzbarsky) → review+

Comment 28

12 years ago
Comment on attachment 209518 [details] [diff] [review]
part II: Make uriloader handle this better

biesi: this doesn't seem to do anything.  (true && something) is true if something is true.  what is the point of aChannelMayBeOpen?  or, is there another patch yet to come?
Comment on attachment 209232 [details] [diff] [review]
part I: Fix venkman (checked in)

venkman patch checked in
Checking in js/venkman-service.js;
/cvsroot/mozilla/extensions/venkman/js/venkman-service.js,v  <--  venkman-service.js
new revision: 1.13; previous revision: 1.12
done
Comment on attachment 209518 [details] [diff] [review]
part II: Make uriloader handle this better

yes, I missed to patch the other caller. I must've done something wrong when I tested this. plus, this function could be written better anyway.
Attachment #209518 - Flags: superreview?(darin) → superreview-
Created attachment 209590 [details] [diff] [review]
part III: improve nsIRequest docs, v2 (checked in)

this is the nsIRequest patch that I checked in

Checking in nsIRequest.idl;
/cvsroot/mozilla/netwerk/base/public/nsIRequest.idl,v  <--  nsIRequest.idl
new revision: 1.15; previous revision: 1.14
done
Attachment #209528 - Attachment is obsolete: true
Created attachment 209652 [details] [diff] [review]
part II: Make uriloader handle this better, v2 (checked in)

ok, this version should be correct.
Attachment #209518 - Attachment is obsolete: true
Attachment #209652 - Flags: superreview?(darin)
Attachment #209652 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9alpha

Updated

12 years ago
Attachment #209652 - Flags: superreview?(darin) → superreview+
Comment on attachment 209652 [details] [diff] [review]
part II: Make uriloader handle this better, v2 (checked in)

Nice.
Attachment #209652 - Flags: review?(bzbarsky) → review+
Attachment #209232 - Attachment description: part I: Fix venkman → part I: Fix venkman (checked in)
Comment on attachment 209652 [details] [diff] [review]
part II: Make uriloader handle this better, v2 (checked in)

this, too, is now checked in
Checking in uriloader/base/nsURILoader.cpp;
/cvsroot/mozilla/uriloader/base/nsURILoader.cpp,v  <--  nsURILoader.cpp
new revision: 1.139; previous revision: 1.138
done
Checking in uriloader/base/nsURILoader.h;
/cvsroot/mozilla/uriloader/base/nsURILoader.h,v  <--  nsURILoader.h
new revision: 1.26; previous revision: 1.25
done
Attachment #209652 - Attachment description: part II: Make uriloader handle this better, v2 → part II: Make uriloader handle this better, v2 (checked in)
fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Component: JavaScript Debugger → Embedding: Docshell
Product: Other Applications → Core
Comment on attachment 209590 [details] [diff] [review]
part III: improve nsIRequest docs, v2 (checked in)

I think these improved docs would be nice on the 1.8.1 branch.
Attachment #209590 - Flags: branch-1.8.1?(darin)

Updated

12 years ago
Attachment #209590 - Flags: branch-1.8.1?(darin) → branch-1.8.1+
nsIRequest docs checked in on MOZILLA_1_8_BRANCH
Checking in netwerk/base/public/nsIRequest.idl;
/cvsroot/mozilla/netwerk/base/public/nsIRequest.idl,v  <--  nsIRequest.idl
new revision: 1.14.28.1; previous revision: 1.14
done
You need to log in before you can comment on or make changes to this bug.