Closed Bug 646509 Opened 9 years ago Closed 9 years ago

proxyserver with PAC doesn't work in latest nightly

Categories

(Core :: General, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5
Tracking Status
blocking-fx --- ?

People

(Reporter: jo.hermans, Assigned: Waldo)

References

Details

(Keywords: regression)

Attachments

(2 files)

Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110330 Firefox/4.2a1pre

I noticed that the latest nightly doesn't work behind a proxyserver with a PAC-file.

The console mentions this error, but I can't remember if I saw it working earlier today (the proxyserver worked, but I didn't look at the console) :

Error: XPCSafeJSObjectWrapper is not defined
Source File: resource://gre/components/nsProxyAutoConfig.js
Line: 98

I'm a bit busy right now, but I'll try to look into it later tonight.
Could this be fallout from the TM merge?
http://hg.mozilla.org/mozilla-central/rev/e05523998f2f

It seems that js/src/xpconnect/src/XPCSafeJSObjectWrapper.cpp got removed in bug 580128, but that's from October last year. I don't know why this error suddenly appears - maybe we're triggering a different exception after the TM merge ?
I'm also in a corp. env. with a proxy pac file to go out, see img: http://666kb.com/i/bsgmzzdvgocqa07se.jpg

and it does work!
have you used 4.2a1pre build?
I too am on the other side of a corporate proxy and the proxy PAC file works for me with 4.2a1pre (both 7 & 8 April builds have worked).
Bug 648516 seems to indicate that it does exists ...

Hmmm .. the bug happens when some kind of exception is trying to be reported. Maybe not everyone has the same error, it could depend on the PAC file.
Attached file misbehaving PAC file
This is my PAC file. I know, it's very large :-(

It uses the shExpMatch(), isPlainHostName(), isInNet() and localHostOrDomainIs() functions.
Duplicate of this bug: 648516
(In reply to comment #7)
> Created attachment 524619 [details]
> misbehaving PAC file
> 
> This is my PAC file. I know, it's very large :-(
> 
> It uses the shExpMatch(), isPlainHostName(), isInNet() and
> localHostOrDomainIs() functions.

Mine starts working if I remove our single use of isInNet():

isInNet(host, "10.0.0.0",  "255.0.0.0")
isInNet calls a regular expression as a function, bug 582717 made that not work anymore
Will, thanks!

Jeff, can you please fix?
Assignee: nobody → jwalden+bmo
Blocks: 582717
blocking-fx: --- → ?
Sure.
Status: NEW → ASSIGNED
Attached patch PatchSplinter Review
The JS pit rubber-stamped this, doesn't seem to require any more area-specific once-overs to me.
Attachment #524695 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/272a7b90280d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
and the reference to XPCSafeJSObjectWrapper (which doesn't exists anymore), shouldn't be fixed too ?
Verified as fixed in build 4.2a1pre (2011-4-10).

Klaus does it work for you too ?
Does this code have tests?
Flags: in-testsuite?
This seems to have regressed: I'm currently using 6.0.2 on 32-bit Windows XP and, per comment 6, a runtime error/exception in the .pac file...

  function FindProxyForURL(url, host) { nosuchfunction(); return 'DIRECT'; }

...causes the error:

  Error: XPCSafeJSObjectWrapper is not defined
  Source File: resource:///components/nsProxyAutoConfig.js
  Line: 98

netwerk/base/src/nsProxyAutoConfig.js in my 6.0.1 sandbox:

94-        // Call the original function
95-        try {
96-            var rval = this._sandBox.FindProxyForURL(testURI, testHost);
97-        } catch (e) {
98:            throw XPCSafeJSObjectWrapper(e);
99-        }

Comment 15 said that this should have been removed...?
XPCSafeJSObjectWrapper doesn't exist any more.
Then it sounds like whoever removed it didn't do it quite right...  Would just throwing |e| there do the right thing (or removing the try/catch altogether)?
Yeah the sandbox should do the right wrapping.
According to MXR, there are 9 lines containing the string "XPCSafeJSObjectWrapper" that remain: http://mxr.mozilla.org/mozilla-central/search?string=XPCSafeJSObjectWrapper
Except for the ref in Comment 18, looks like they are all in tests, comments or string constants.
I filed bug 687561 on the XPCSafeJSObjectWrapper thing, but the underlying problem is the exception being thrown, no?

Shane, can you possibly hunt down via nightlies when the problem reappeared for you?  And maybe file a new bug on it, so we can track it clearly?
Sorry about the confusion, but comment 18 is just XPCSafeJSObjectWrapper:

I introduced an error in my proxy.pac file (my fault), but the error messages reported this XPCSafeJSObjectWrapper thing and *not* the error in my proxy.pac file.

Further, although I (mistakenly) said this is a regression, I now think that this bug was never actually fixed! Comment 15 suggests that Jo thought that it was not yet fixed; comment 16, that although Jo "verified" it, he wasn't 100% sure that it was fixed (i.e. he asks Klaus to confirm); and comment 17, that nobody ever added tests.

I'd be inclined to reopen this bug, mark bug 687561 as a dup of it, and consider it fixed when a user error in proxy.pac is reported correctly -- maybe with tests this time. :) Or you can just use bug 687561 for that purpose, if you like.

I don't think I have anything further to contribute at this time, except maybe to verify that the thing is indeed fixed. :)
Shane, this bug was about a bug in nsProxyAutoConfig.js that caused it to throw exceptions with _correct_ PAC files.  Due to bug 687561 those exceptions claimed to be about XPCSafeJSObjectWrapper instead of being about calling a RegExp object.  See comment 10 for a description of the problem.

Comment 15 points out that while the bug in the code that caused it to throw exceptions was fixed there was another bug, which is that exceptions from PAC files, whether caused by bugs in the PAC file itself or in nsProxyAutoConfig.js are reported in a useless way.  Sadly, no one filed a bug report on that second bug.  That has now been filed as bug 687561.

So this bug as filed was in fact fixed.  Jo verified that the fix fixed the problem he was seeing, and since he _reported_ this bug, he was in a pretty good position to judge!  He asked Klaus to confirm that the fix also fixed the problem _Klaus_ was seeing, which had been reported as a separate bug 648516, which was then marked a duplicate of this one (see the duplicates list).  This was asked on the off chance that Klaus had in fact been seeing a different problem from the (fixed) problem Jo was seeing, in which case his bug would not have been a duplicate and should have been reopened.  Given that Klaus did not, in fact, reopen bug 648516, I think it's safe to say his issue was in fact fixed with the patch in this bug.

> I'd be inclined to reopen this bug

I'm glad you resisted this erroneous inclination.

> mark bug 687561 as a dup of it,

Likewise.  I recommend not doing that sort of thing to other people's bug reports unless you are _very_ familiar with the bug system, the code involved, or both.  ;)

It sounds like the issyou _you_ ran into is just bug 687561, which we should indeed fix.
> issyou

issue, of course.  It's clearly too late at night....
As far as I can tell, the UI won't let me do either of those things; but even if it did, I wouldn't because I'm not familiar with the rules around here (perhaps why the UI won't let me do those things, eh?).

Anyway, you're right that comment 15 is different. Bug 687561 it is!
You need to log in before you can comment on or make changes to this bug.