646509 - proxyserver with PAC doesn't work in latest nightly

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Jo Hermans]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Could this be fallout from the TM merge?

Comment 2

[Jo Hermans]

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 ?

Comment 3

[Ixle Pivués]

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!

Comment 4

[Alessandro]

have you used 4.2a1pre build?

Comment 5

[Al Ferguson]

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

Comment 6

[Jo Hermans]

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.

Comment 7

[Jo Hermans]

Attachment: misbehaving PAC file

This is my PAC file. I know, it's very large :-(

It uses the shExpMatch(), isPlainHostName(), isInNet() and localHostOrDomainIs() functions.

Comment 9

[Will Roberts [:bws42]]

(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")

Comment 10

[Will Roberts [:bws42]]

isInNet calls a regular expression as a function, bug 582717 made that not work anymore

Comment 11

[Boris Zbarsky [:bzbarsky]]

Will, thanks!

Jeff, can you please fix?

Comment 12

[Jeff Walden [:Waldo]]

Sure.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Patch

The JS pit rubber-stamped this, doesn't seem to require any more area-specific once-overs to me.

Comment 14

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/272a7b90280d

Comment 15

[Jo Hermans]

and the reference to XPCSafeJSObjectWrapper (which doesn't exists anymore), shouldn't be fixed too ?

Comment 16

[Jo Hermans]

Verified as fixed in build 4.2a1pre (2011-4-10).

Klaus does it work for you too ?

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Does this code have tests?

Comment 18

[Shane Beasley]

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

Comment 19

[Andreas Gal :gal]

XPCSafeJSObjectWrapper doesn't exist any more.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

Comment 21

[Andreas Gal :gal]

Yeah the sandbox should do the right wrapping.

Comment 22

[Andrew McCreight [:mccr8]]

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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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?

Comment 24

[Shane Beasley]

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

Comment 25

[Boris Zbarsky [:bzbarsky]]

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.

Comment 26

[Boris Zbarsky [:bzbarsky]]

> issyou

issue, of course.  It's clearly too late at night....

Comment 27

[Shane Beasley]

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!