Last Comment Bug 750445 - Fix for bug 235853 landed broken: unused variable in LookupProxyInfo.
: Fix for bug 235853 landed broken: unused variable in LookupProxyInfo.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: x86_64 Linux
: -- normal with 5 votes (vote)
: mozilla17
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on: 235853 723628 767005
Blocks: 745296 769764
  Show dependency treegraph
 
Reported: 2012-04-30 13:15 PDT by Jason Duell [:jduell] (needinfo? me)
Modified: 2012-08-09 08:12 PDT (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
v1 (1.42 KB, patch)
2012-04-30 13:15 PDT, Jason Duell [:jduell] (needinfo? me)
no flags Details | Diff | Review

Description Jason Duell [:jduell] (needinfo? me) 2012-04-30 13:15:03 PDT
Created attachment 619662 [details] [diff] [review]
v1

Yay for WARNINGS_AS_ERRORS.  It just turned up that we're not actually using the 'flags' variable we set in LookupProxyInfo().  
 
This code was recently refactored by Patrick (bug 723628), but the actual bug has been sitting there since bug 235853:

  http://hg.mozilla.org/mozilla-central/diff/20240add1d35/netwerk/base/src/nsIOService.cpp

We set flags=RESOLVE_NON_BLOCKING but don't use it.  This appears to be a result of jdm's rebase here:

  https://bugzilla.mozilla.org/attachment.cgi?id=594181&action=diff

The original patch from jrmuizel used the flags variable, but ignored the aProxyFlags parameter.  But that seems wrong to me too:

   https://bugzilla.mozilla.org/attachment.cgi?id=346370&action=diff

Seems like we should be adding the non-blocking flag to the passed-in flags.  Otherwise we are basically dropping the passed-in flags on the floor. (We only have one C++ client--websockets--that calls this with flags: I suspect this may be causing bug 713026)

I assume this means that we're still blocking the main thread for PAC DNS resolution? Did we ever actually test that? :(
Comment 1 Jason Duell [:jduell] (needinfo? me) 2012-04-30 15:01:39 PDT
Correction: we're using the passed-in flags now, so this isn't causing bug 713026.  But it we start ignoring them, it will make that bug more broken :)
Comment 2 Jason Duell [:jduell] (needinfo? me) 2012-05-01 22:11:36 PDT
Comment on attachment 619662 [details] [diff] [review]
v1

clearing review--this little patch somehow messes up most tests in test_websocket.html.
Comment 3 gak 2012-07-18 07:27:20 PDT
Is there any update on this?  Can it please get fixed soon?
Comment 4 Jason Duell [:jduell] (needinfo? me) 2012-07-18 10:32:20 PDT
Yes, we've had a recent eruption of proxy-related bugs which we are plowing through, and this is on that list.
Comment 5 gak 2012-07-20 07:08:11 PDT
thank you for the update Jason.  is there a target release or timeframe?
Comment 6 Patrick McManus [:mcmanus] 2012-07-24 10:55:59 PDT
Comment on attachment 619662 [details] [diff] [review]
v1

Review of attachment 619662 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/src/nsIOService.cpp
@@ +630,1 @@
>          rv = mProxyService->Resolve(aProxyURI ? aProxyURI : aURI, aProxyFlags,

hopefully this should just be aProxyFlags |= nsIProtocolProxyService::RESOLVE_NON_BLOCKING;

I'll retest websockets mochi with that.
Comment 7 Patrick McManus [:mcmanus] 2012-07-24 13:55:19 PDT
The unused variable basis of this bug report will be cured by the backout that will be done by 767005.

The portions of this bug report that deal with removing blocking from PAC file usage will be undertaken in bug 769764.
Comment 8 Patrick McManus [:mcmanus] 2012-07-25 08:15:09 PDT
fixed by 767005
Comment 9 Ioana (away) 2012-08-09 08:12:55 PDT
Patrick, is there anything QA can do to verify this fix?

Note You need to log in before you can comment on or make changes to this bug.