Closed
Bug 1233360
Opened 9 years ago
Closed 8 years ago
proxy bypass ignored on Mac when using system proxy settings
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: persiantools, Assigned: xeonchen)
References
Details
(Whiteboard: [necko-active])
Attachments
(3 files)
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_2) AppleWebKit/601.3.9 (KHTML, like Gecko) Version/9.0.2 Safari/601.3.9 Steps to reproduce: Firefox 43 on Mac OS X El-Capitan ignores system proxy bypass settings when it is using system proxy settings. I have set http and https proxies in Mac network preferences and the default proxy bypass string is *.local, 169.254/16, 192.168/16 This works flawlessly in Safari but Firefox fails to connect to local sites in 192.168.* network. On the other hand, Firefox fully honours proxy bypass patterns described in Automatic Network Configuration scripts. This issue seems to be Mac specific and it is not reproducible on Windows. On an interesting note, I also found a similar issue 470207 which was resolved back in 2009. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:43.0) Gecko/20100101 Firefox/43.0
Updated•8 years ago
|
Component: Untriaged → Networking
Product: Firefox → Core
Updated•8 years ago
|
Whiteboard: [necko-backlog]
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → xeonchen
Whiteboard: [necko-backlog] → [necko-active]
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50697/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50697/
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50699/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50699/
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/50701/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/50701/
Assignee | ||
Comment 4•8 years ago
|
||
We didn't handle the form '<IP>/<prefix>', add implementation and tests here.
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8749023 [details] MozReview Request: Bug 1233360 - Part 1: Move out OSX bypass rules checker; r?bagder Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50697/diff/1-2/
Attachment #8749023 -
Attachment description: MozReview Request: Bug 1233360 - Part 1: Move out OSX bypass rules checker; r?badger → MozReview Request: Bug 1233360 - Part 1: Move out OSX bypass rules checker; r?bagder
Attachment #8749024 -
Attachment description: MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?badger → MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder
Attachment #8749025 -
Attachment description: MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?badger → MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?bagder
Attachment #8749023 -
Flags: review?(daniel)
Attachment #8749024 -
Flags: review?(daniel)
Attachment #8749025 -
Flags: review?(daniel)
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8749024 [details] MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50699/diff/1-2/
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8749025 [details] MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?bagder Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50701/diff/1-2/
Comment 8•8 years ago
|
||
Comment on attachment 8749023 [details] MozReview Request: Bug 1233360 - Part 1: Move out OSX bypass rules checker; r?bagder https://reviewboard.mozilla.org/r/50697/#review48017 ::: toolkit/system/osxproxy/moz.build:12 (Diff revision 2) > SOURCES += [ > 'nsOSXSystemProxySettings.mm', > ] > > +UNIFIED_SOURCES += [ > + 'ProxyUtils.cpp', This makes it a lovely mix of .cpp and .mm files here. Did you make a decision here or how come the code extracted from a .mm got to be a .cpp? I'm not aware of how we've decided what to use where, I'm just noticing the inconsistency here.
Attachment #8749023 -
Flags: review?(daniel)
Updated•8 years ago
|
Attachment #8749024 -
Flags: review?(daniel)
Comment 9•8 years ago
|
||
Comment on attachment 8749024 [details] MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder https://reviewboard.mozilla.org/r/50699/#review48019 ::: toolkit/system/osxproxy/ProxyUtils.cpp:16 (Diff revision 2) > namespace mozilla { > namespace toolkit { > namespace system { > > -bool IsHostProxyEntry(const nsACString& aHost, const nsACString& aOverride) > +static bool > +IPv4Parser(const nsACString& aAddr, uint32_t& ipv4) Why have your own implemenation that looks pretty much like a subset of inet_pton() ?
Assignee | ||
Comment 10•8 years ago
|
||
https://reviewboard.mozilla.org/r/50699/#review48019 > Why have your own implemenation that looks pretty much like a subset of inet_pton() ? Yes, but we'll have to handle the form like '10/8' instead of '10.0.0.0/8'. Also the result of inet_pton can't be masked directly, or is there a better solution?
Assignee | ||
Comment 11•8 years ago
|
||
https://reviewboard.mozilla.org/r/50697/#review48017 > This makes it a lovely mix of .cpp and .mm files here. Did you make a decision here or how come the code extracted from a .mm got to be a .cpp? > > I'm not aware of how we've decided what to use where, I'm just noticing the inconsistency here. I expect this function would be reused by other platforms. But you're right, this should be done when it have to be.
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8749023 [details] MozReview Request: Bug 1233360 - Part 1: Move out OSX bypass rules checker; r?bagder Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50697/diff/2-3/
Attachment #8749023 -
Flags: review?(daniel)
Attachment #8749024 -
Flags: review?(daniel)
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8749024 [details] MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50699/diff/2-3/
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8749025 [details] MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?bagder Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50701/diff/2-3/
Assignee | ||
Comment 15•8 years ago
|
||
https://reviewboard.mozilla.org/r/50699/#review48019 > Yes, but we'll have to handle the form like '10/8' instead of '10.0.0.0/8'. > Also the result of inet_pton can't be masked directly, or is there a better solution? Hi bagder, I've just made some modification, would you please see if it looks good to you?
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8749024 [details] MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50699/diff/3-4/
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8749025 [details] MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?bagder Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50701/diff/3-4/
Comment 18•8 years ago
|
||
Comment on attachment 8749023 [details] MozReview Request: Bug 1233360 - Part 1: Move out OSX bypass rules checker; r?bagder https://reviewboard.mozilla.org/r/50697/#review49041
Attachment #8749023 -
Flags: review?(daniel) → review+
Updated•8 years ago
|
Attachment #8749024 -
Flags: review?(daniel) → review+
Comment 19•8 years ago
|
||
Comment on attachment 8749024 [details] MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder https://reviewboard.mozilla.org/r/50699/#review49043 Apart the little format nit, it looks fine! ::: toolkit/system/osxproxy/ProxyUtils.mm:58 (Diff revision 4) > + > + if (aMaskLen > 96) { > + aAddr.pr_s6_addr32[3] = PR_htonl( > + PR_ntohl(aAddr.pr_s6_addr32[3]) & (~0L << (128 - aMaskLen))); > + } > + else if (aMaskLen > 64) { Nit: our style guide says we write them '} else if' on the same line. You have this on several places.
Comment 20•8 years ago
|
||
Comment on attachment 8749025 [details] MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?bagder https://reviewboard.mozilla.org/r/50701/#review49045
Attachment #8749025 -
Flags: review?(daniel) → review+
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8749024 [details] MozReview Request: Bug 1233360 - Part 2: implement bypass rules checker; r?bagder Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50699/diff/4-5/
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8749025 [details] MozReview Request: Bug 1233360 - Part 3: add tests for OSX proxy bypass rules checker; r?bagder Review request updated; see interdiff: https://reviewboard.mozilla.org/r/50701/diff/4-5/
Comment 23•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ccbedc9308f https://hg.mozilla.org/integration/mozilla-inbound/rev/81add9b7d007 https://hg.mozilla.org/integration/mozilla-inbound/rev/1b83aa3ab364
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4ccbedc9308f https://hg.mozilla.org/mozilla-central/rev/81add9b7d007 https://hg.mozilla.org/mozilla-central/rev/1b83aa3ab364
Status: UNCONFIRMED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•