Closed Bug 86846 Opened 23 years ago Closed 21 years ago

PAC: alert() logged in JavaScript Console

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.6beta

People

(Reporter: mkaply, Assigned: darin.moz)

References

Details

(Keywords: helpwanted, Whiteboard: checklinux)

Attachments

(2 files, 6 obsolete files)

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.
Keywords: 4xp
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
Blocks: 79893
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?
QA Contact: benc → pacqa
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. 
We could make alert equal to dump...
i'd prefer for us to use the jsconsole logging features...
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
alert == window.alert, and I don't know if its possible to arrange for the
sandbox to have access to the current window.
What about Javascript console? Would that be feasible?
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.
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.
Blocks: 93845
sandbox dump() is wired to stderr, see bug 85290 about removing pac console 
logging in release build
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.
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.
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.
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.
Attached patch first alert() patch (obsolete) — Splinter Review
bbaetz, could you r= the patch, please.
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?
>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.




Attached patch improved patch (obsolete) — Splinter Review
I checked about security concerns, and there are none for opening an alert
window via PAC.
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
to default owner
Assignee: serge → new-network-bugs
+helpwanted, mozilla 1.0, patch
qa to me.

Clear problem description, patch ready.
QA Contact: pacqa → benc
*** Bug 137580 has been marked as a duplicate of this bug. ***
I gave r= to this back in Aug - assuming it still applies, it just needs sr.
Keywords: nsbeta1
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 → ---
Target Milestone: --- → Future
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 on attachment 46008 [details] [diff] [review]
improved patch

darin, dougt?
Attachment #46008 - Flags: superreview?(darin)
Attachment #46008 - Flags: review?(dougt)
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?
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-
Attachment #46008 - Flags: review?(dougt)
Any news on this one?
adt: nsbeta1-
Keywords: nsbeta1nsbeta1-
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.
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.... 
Attached patch updated patch (obsolete) — Splinter Review
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 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 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
1. sounds fine
2. yeah i meant to mention that but i was in a hurry
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.
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-
Attachment #133981 - Flags: review?(altitude)
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 ;-)
Attachment #133981 - Attachment is obsolete: true
Attachment #134070 - Flags: review?(timeless)
-> me
Assignee: new-network-bugs → darin
Target Milestone: Future → mozilla1.6beta
Attachment #134070 - Attachment is obsolete: true
Attachment #134070 - Flags: review?(timeless)
Attachment #134071 - Flags: superreview?(bzbarsky)
Attachment #134071 - Flags: review?(cbiesinger)
Attachment #134071 - Flags: review?(cbiesinger) → review+
Comment on attachment 134071 [details] [diff] [review]
v1.2 patch - just use logStringMessage (thanks biesi)

sr=bzbarsky
Attachment #134071 - Flags: superreview?(bzbarsky) → superreview+
Status: NEW → ASSIGNED
Whiteboard: [ready-to-land]
Ah.  This worked like a charm (the v 1.2 patch).  

I hope this makes it into the next rev.  

Thanks.  
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
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
V/fixed, mozilla 1.7.2/xp
Whiteboard: checklinux checkwin → checklinux
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: