Closed
Bug 86846
Opened 23 years ago
Closed 21 years ago
PAC: alert() logged in JavaScript Console
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
VERIFIED
FIXED
mozilla1.6beta
People
(Reporter: mkaply, Assigned: darin.moz)
References
Details
(Keywords: helpwanted, Whiteboard: checklinux)
Attachments
(2 files, 6 obsolete files)
1.66 KB,
patch
|
Biesinger
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
216 bytes,
text/plain
|
Details |
The following autoproxy config file does not display an alert: function FindProxyForURL(url, host) { alert(host); alert(url); return "DIRECT"; } In 4.x alerts are displayed, and this is very helpful for debugging problems in the autoproxy config file.
Lots of javascript apparently worked in Nav4 PAC, but we are only implementing a specific set. You might want to post to the netlib newsgroup and see what people say...
Summary: Cannot display alert() in autproxy config file → PAC:Cannot display alert() in autproxy config file
Comment 2•23 years ago
|
||
I saw the post in n.p.m.netlib, but I didn't see any followups :\ Michael, I've noticed that in debug/cvs builds dump() statements I insert into PAC files will execute. Does this help at all?
Comment 3•23 years ago
|
||
Serge, can you take a look at these PAC issues? Thanks - Jussi-Pekka
Assignee: jpm → serge
Unless it creates a security problem, we should add this. Console logging of PAC is not coming back anytime soon.
Comment 5•23 years ago
|
||
We could make alert equal to dump...
Reporter | ||
Comment 7•23 years ago
|
||
As long as it goes somewhere that works by default in a retail build - dump does not. They key here is that people used to have a nice ability to debug PAC files - now they don't
Comment 8•23 years ago
|
||
alert == window.alert, and I don't know if its possible to arrange for the sandbox to have access to the current window.
Reporter | ||
Comment 9•23 years ago
|
||
What about Javascript console? Would that be feasible?
Comment 10•23 years ago
|
||
Sure, it's possible to implement additional function for js console output, but I do not understand what prevents the pac developers to use js dump() call? I think dump() is perfectly suitable for debug output, on on any unix flavor js dump() works just fine in release build, on windows you have to start mozilla.exe -console, I don't know how it works on mac, though.
Comment 11•23 years ago
|
||
dump() on release builds is wired to /dev/null or an equiv. since PAC debugging is done by end users, this isn't accessible. js console sounds great.
Comment 12•23 years ago
|
||
sandbox dump() is wired to stderr, see bug 85290 about removing pac console logging in release build
Comment 13•23 years ago
|
||
Comment 14•23 years ago
|
||
It does log dump() argument as a warning message into js console. I did not find what is the value of flags (6th argument in init() method of nsIScriptError) I have to set to make logging into message panel of js console.
Comment 15•23 years ago
|
||
two concerns, can you wrap the /**/ comment? var ios = Components.classes[kIOSERVICE_CONTRACTID].getService(nsIIOService); var dns = Components.classes[kDNS_CONTRACTID].getService(nsIDNSService); +var cns = Components.classes[kCONSOLESERVICE_CONTRACTID].getService(nsIConsoleService); +var jseo = Components.classes[kSCRIPTERROR_CONTRACTID].createInstance(nsIScriptError); if any of these fail, execution halts. fwiw there's no need to cc me in most cases I'm watching the reporter and perhaps some of the cc's.
Comment 16•23 years ago
|
||
If you are going to add dump, please do so in a separate bug. alert is a 4xp feature, so unless there is a security or marketing consideration, it should be implemented as well.
Comment 17•23 years ago
|
||
Well, if I 'd debug pac files I'd prefer to use dump() output, it gives me good printf() like stream of output messages... Anyway alert's patch is following.
Comment 18•23 years ago
|
||
Comment 19•23 years ago
|
||
Comment 20•23 years ago
|
||
bbaetz, could you r= the patch, please.
Comment 21•23 years ago
|
||
If we can't alert, we should dump the error to the console. Also, you don't need to test against null explicitly, but since the rest of that file does so, its not a big issue. In what situation won't you be able to get the promptService, but will be able to get the parent window?
Comment 22•23 years ago
|
||
>If we can't alert...
agreed.
I've changed the code a little bit, now parentWindow is local var, and it's
getting reassign every time. If there is no promptService I'm not trying to get
a parent window at all.
Thank for the comments. New patch is following.
Comment 23•23 years ago
|
||
Comment 24•23 years ago
|
||
I checked about security concerns, and there are none for opening an alert window via PAC.
Comment 25•23 years ago
|
||
Theres a typo in the first line comment. Assuming that you've tested this in both a mozilla build and an embedding one, r=bbaetz
Comment 27•22 years ago
|
||
+helpwanted, mozilla 1.0, patch qa to me. Clear problem description, patch ready.
QA Contact: pacqa → benc
Comment 28•22 years ago
|
||
*** Bug 137580 has been marked as a duplicate of this bug. ***
Comment 29•22 years ago
|
||
I gave r= to this back in Aug - assuming it still applies, it just needs sr.
Comment 30•22 years ago
|
||
Temporarily "futuring" all PAC&SOCKS bugs to clear new-networking queue. I will review later. (I promise) If you object, and can make a case for a mozilla 1.0 fix, please reset milestone to "--" or email me.
Target Milestone: --- → Future
Keywords: mozilla1.0
QA Contact: benc → pacqa
Summary: PAC:Cannot display alert() in autproxy config file → PAC: Cannot display alert() in autproxy config file
Target Milestone: Future → ---
Updated•22 years ago
|
Target Milestone: --- → Future
Comment 31•22 years ago
|
||
Any chance of revisiting this soon? I'd like to be able to use alert(); it was hard to find even the alert() debugging technique (I'm a .pac user, not a JS developer). dump() works, so I'm glad I found this bug, but alert() is the best-known debugging method for .pacs.
Comment 32•22 years ago
|
||
Comment on attachment 46008 [details] [diff] [review] improved patch darin, dougt?
Attachment #46008 -
Flags: superreview?(darin)
Attachment #46008 -
Flags: review?(dougt)
Assignee | ||
Comment 33•22 years ago
|
||
Comment on attachment 46008 [details] [diff] [review] improved patch >Index: nsProxyAutoConfig.js > var LocalFindProxyForURL=null; > // sendbox in which we eval loaded autoconfig js file > var ProxySandBox = null; >+// global vars for alert() >+var promptService = null; >+var windowManager = null; >+var promptServiceAvailable = true; nit: would be nice to use the same naming convention for all global vars. that said, it seems like the file already has mixed conventions, so i'm not going to press too hard to make them the same. but, if you're willing to do the extra work it would be good to fix nsProxyAutoConfig.js to use a consistent naming convention for globals ;-) >+ var parentWindow = windowManager.getMostRecentWindow("mozilla:preferences"); >+ if (!parentWindow) { >+ parentWindow = windowManager.getMostRecentWindow("navigator:browser"); so we don't allow PAC to execute an alert when only mailnews is visible? clearly we can execute the PAC file when only mailnews is visible, so isn't this a problem/bug? is there no way to just say: give me the topmost window?
Assignee | ||
Comment 34•22 years ago
|
||
Comment on attachment 46008 [details] [diff] [review] improved patch setting sr- to get this off my review request queue.
Attachment #46008 -
Flags: superreview?(darin) → superreview-
Updated•22 years ago
|
Attachment #46008 -
Flags: review?(dougt)
Comment 35•22 years ago
|
||
Any news on this one?
Comment 37•21 years ago
|
||
So what's the status of this? It's now 2 years later, and "alert()" still doesn't work in moz 1.5 (windows). dump() doesn't seem to work either. This kinda sucks because there is no way to debug .pac files.
Assignee | ||
Comment 38•21 years ago
|
||
alex: please note the helpwanted keyword.... unfortunately, no one has had time to finish off the patch :( seems like there's good stuff in this bug....
Comment 39•21 years ago
|
||
Attachment #45117 -
Attachment is obsolete: true
Attachment #45481 -
Attachment is obsolete: true
Attachment #45482 -
Attachment is obsolete: true
Attachment #46008 -
Attachment is obsolete: true
Comment 40•21 years ago
|
||
Comment on attachment 133981 [details] [diff] [review] updated patch Alex Tang: does this work? if so please mark r+ Darin: i should note that you didn't follow / enforce conventions for tingley@sundell.net's patch for bug 93924.
Attachment #133981 -
Flags: superreview?(darin)
Attachment #133981 -
Flags: review?(altitude)
Comment 41•21 years ago
|
||
Comment on attachment 133981 [details] [diff] [review] updated patch 1. why not pass nsnull to PromptService.alert? either always, or if you don't get a parentWindow 2. Should "PAC: Alert" be localizable? And maybe "Proxy Autoconfiguration: Alert" may be a better title
Comment 42•21 years ago
|
||
1. sounds fine 2. yeah i meant to mention that but i was in a hurry
Comment 43•21 years ago
|
||
Alex: if you have a PAC file that does not work, you can send it to me, or open a bug and attach the file.
Assignee | ||
Comment 44•21 years ago
|
||
Comment on attachment 133981 [details] [diff] [review] updated patch Index: mozilla/netwerk/base/src/nsProxyAutoConfig.js >+function proxyAlert(msg) { >+ if (!PromptService && PromptServiceAvailable) { //going in just ones you mean "going in just once" or do you mean something else? >+ if (PromptServiceAvailable) { >+ try { >+ var parentWindow = WindowManager.getMostRecentWindow("mozilla:preferences"); >+ if (!parentWindow) { >+ parentWindow = WindowManager.getMostRecentWindow("navigator:browser"); >+ } is this going to work well in firebird or thunderbird? or what about standalone composer? why bother trying to correctly parent this window... it is just for error reporting. so, let's just keep things simple. if i were writing this patch, i would just get the promptservice and call alert with a null parent window. that would _so_ simply this proxyAlert function.
Attachment #133981 -
Flags: superreview?(darin) → superreview-
Assignee | ||
Updated•21 years ago
|
Attachment #133981 -
Flags: review?(altitude)
Assignee | ||
Comment 45•21 years ago
|
||
ok, it turns out that putting up a modal dialog inside PAC causes crashes and other badness in layout. the problem is that the caller of NewChannel (which calls into PAC) may not expect PLEvents to be dispatched inside that call, which is what happens when a modal dialog is thrown up. using the JS console is a nice way to get around this problem, and IMO it is a better solution anyways. alert dialogs are a pain anyways, especially if the PAC file wants to generate many of them -- how many times do you have to press OK before you get tired and just kill the process?!? so, this patch goes back to the original first patch submitted by serge and just cleans it up a bit. hence, this patch is version 1.1 ;-)
Assignee | ||
Updated•21 years ago
|
Attachment #133981 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #134070 -
Flags: review?(timeless)
Assignee | ||
Comment 46•21 years ago
|
||
-> me
Assignee: new-network-bugs → darin
Target Milestone: Future → mozilla1.6beta
Assignee | ||
Comment 47•21 years ago
|
||
Attachment #134070 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #134070 -
Flags: review?(timeless)
Assignee | ||
Updated•21 years ago
|
Attachment #134071 -
Flags: superreview?(bzbarsky)
Attachment #134071 -
Flags: review?(cbiesinger)
Updated•21 years ago
|
Attachment #134071 -
Flags: review?(cbiesinger) → review+
Comment 48•21 years ago
|
||
Comment on attachment 134071 [details] [diff] [review] v1.2 patch - just use logStringMessage (thanks biesi) sr=bzbarsky
Attachment #134071 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Updated•21 years ago
|
Status: NEW → ASSIGNED
Whiteboard: [ready-to-land]
Comment 49•21 years ago
|
||
Ah. This worked like a charm (the v 1.2 patch). I hope this makes it into the next rev. Thanks.
Assignee | ||
Comment 50•21 years ago
|
||
fixed-on-trunk (for 1.6 beta) benc: support for alert throwing up a modal dialog is WONTFIX, so i'm marking this bug fixed.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Comment 51•21 years ago
|
||
VERIFIED, Mac OS X 1.6b. updated bug, cleared comments. Darin: there is no spec that says alert must display dialogs, but I do remember some customers from the Netscape days which used alert. I was never a huge fan of this, I wanted URL re-direction instead, which would have been much more flexible for user interactions and errors. If it can't be done, it can't be done!
Status: RESOLVED → VERIFIED
Summary: PAC: Cannot display alert() in autproxy config file → PAC: alert() logged in JavaScript Console
Whiteboard: [ready-to-land] → checklinux checkwin
Comment 52•21 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•