Closed
Bug 766973
Opened 12 years ago
Closed 1 year ago
don't allow synchronous DNS queries from the main thread
Categories
(Core :: Networking: DNS, defect, P3)
Core
Networking: DNS
Tracking
()
RESOLVED
DUPLICATE
of bug 1398646
mozilla19
People
(Reporter: jaas, Unassigned)
References
(Depends on 1 open bug)
Details
(Keywords: addon-compat, dev-doc-needed, Whiteboard: [Snappy:P1][necko-backlog])
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
We should try not to allow synchronous DNS queries from the main thread. Not sure if we can get away with it (might break extensions), but we should try and if we succeed we can try to eliminate the API entirely.
try server run
https://tbpl.mozilla.org/?tree=Try&rev=380a095c205d
Attachment #635311 -
Flags: review?(sworkman)
Comment 2•12 years ago
|
||
Comment on attachment 635311 [details] [diff] [review]
fix v1.0
Review of attachment 635311 [details] [diff] [review]:
-----------------------------------------------------------------
Good idea Josh - let's see if if sticks.
Attachment #635311 -
Flags: review?(sworkman) → review+
Found three synchronous resolve users that need to be fixed:
http://mxr.mozilla.org/mozilla-central/source/extensions/auth/nsAuthSSPI.cpp
http://mxr.mozilla.org/mozilla-central/source/netwerk/socket/nsSOCKSIOLayer.cpp
http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit/test_dns_service.js
This is the same thing but with another warning that always happens for sync resolution, main thread or not. It warns that the API might go away.
Attachment #635311 -
Attachment is obsolete: true
Comment 5•12 years ago
|
||
Looks good to me. You can carry over the r+.
I think there is also synchronous DNS resolution in this test:
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsProxyAutoConfig.js
Comment 7•12 years ago
|
||
That's not a test, that's PAC... and that's hard to fix
Update API documentation.
Attachment #635478 -
Attachment is obsolete: true
Attachment #637990 -
Attachment is obsolete: true
Reporter | ||
Comment 10•12 years ago
|
||
Well, that try run is going to fail since Patrick's proxy patches got backed out.
Comment 11•12 years ago
|
||
Comment on attachment 662140 [details] [diff] [review]
fix v1.3
+ NS_WARNING("Synchronous DNS resolve failing - not allowed on the main thread!");
Use NS_ERROR instead?
Reporter | ||
Comment 12•12 years ago
|
||
Attachment #662140 -
Attachment is obsolete: true
Comment 13•12 years ago
|
||
Comment on attachment 665900 [details] [diff] [review]
fix v1.4
Delegating review to Steve.
Attachment #665900 -
Flags: review?(sworkman)
Reporter | ||
Comment 14•12 years ago
|
||
Comment on attachment 665900 [details] [diff] [review]
fix v1.4
Canceling review, Steve already gave the patch r+. Thanks for delegating Jason, I wanted to have that noted so that the review is good enough for check-in.
Attachment #665900 -
Flags: review?(sworkman)
Reporter | ||
Comment 15•12 years ago
|
||
pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/02a62c14ec3b
Comment 16•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Updated•12 years ago
|
Depends on: 815989
Keywords: addon-compat,
dev-doc-needed
Comment 17•12 years ago
|
||
804605 required backing 767158 out of firefox 18 which in turn requires backing this out of 18. 767158 is a pretty lightly used path so it shouldn't have a practical impact on the jank-o-meter.
Target Milestone: mozilla18 → mozilla19
Comment 18•12 years ago
|
||
I've taken 767158 out of the tree completely pending a better fix for 804605. that requires reopening 766973. The only known offender of DNS on the main thread is in conjunction windows integrated auth and involves looking up a different record type for a hostname we have already successfully resolved (in order to connect to) so it should have minimal impact. nonetheless, that's something to get fixed so we can remove the synchronous API.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 19•12 years ago
|
||
Assigning to Patrick since he is at least following the progress of blocking work more than I am.
Assignee: joshmoz → mcmanus
Updated•9 years ago
|
Assignee: mcmanus → nobody
Whiteboard: [Snappy:P1] → [Snappy:P1][necko-backlog]
Comment 20•7 years ago
|
||
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Comment 21•7 years ago
|
||
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
Updated•2 years ago
|
Severity: normal → S3
Comment 22•1 year ago
|
||
This was fixed in bug 1398646.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 1 year ago
Duplicate of bug: 1398646
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•