321932 - Breakpoints do not show up in Source Code view

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Ian Neal]

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.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

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

[Håkan Waara]

Seeing this as well. I'm running trunk version of venkman on OS X, and have tried different themes.

Comment 3

[Ian Neal]

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

Comment 4

[Frank Wein [:mcsmurf]]

Probably Bug 309525 broke this.

Comment 5

[Håkan Waara]

What's the correlation to that bug?

Comment 6

[:Gavin Sharp [email: [e-mail]]]

I was thinking bug 289517 was a likelier candidate.

Comment 7

[Frank Wein [:mcsmurf]]

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).

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Yeah, so.. nsIRequest seems to allow this implementation of isPending. Was that the intent? If so, I should change my uriloader code...

Comment 9

[Boris Zbarsky [:bzbarsky]]

There's no asyncOpen for requests.... :(

Perhaps nsIChannel could document the extra restriction for requests that happen to be channels?

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

well, it could phrase it like "isPending must be true from the time the request started until it completes, and false otherwise" or something. 

Comment 11

[Boris Zbarsky [:bzbarsky]]

Ah, true.  I think that would make sense in general...

Comment 12

[Håkan Waara]

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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

[Håkan Waara]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

There are no patches attached to this bug.  So I have no idea what you're talking about.

Comment 16

[Håkan Waara]

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.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

well... for venkman, it's sufficient. what I really want to do, though, is fix uriloader to handle that case.

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Is this the same issue as bug 321930? Can it be duped or marked as being blocked by this?

Comment 19

[Darin Fisher]

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.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: 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 :-/)

Comment 21

[James Ross]

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).

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

yeah, that should be fine.

Comment 23

[Darin Fisher]

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

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: part II: Make uriloader handle this better

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: part III: Improve nsIRequest docs

this attempts to make the nsIRequest docs a bit clearer

Comment 26

[Darin Fisher]

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."

Comment 27

[Boris Zbarsky [:bzbarsky]]

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.

Comment 28

[Darin Fisher]

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 29

[Christian :Biesinger (don't email me, ping me on IRC)]

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 30

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: 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

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: part II: Make uriloader handle this better, v2 (checked in)

ok, this version should be correct.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209652 [details] [diff] [review]
part II: Make uriloader handle this better, v2 (checked in)

Nice.

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

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

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

fixed.

Comment 36

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 37

[Christian :Biesinger (don't email me, ping me on IRC)]

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