The default bug view has changed. See this FAQ.

don't allow synchronous DNS queries from the main thread

REOPENED
Unassigned

Status

()

Core
Networking: DNS
REOPENED
5 years ago
a year ago

People

(Reporter: Josh Aas, Unassigned)

Tracking

(Depends on: 1 bug, {addon-compat, dev-doc-needed})

Trunk
mozilla19
addon-compat, dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1][necko-backlog])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 635311 [details] [diff] [review]
fix v1.0

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

Comment 1

5 years ago
try server run

https://tbpl.mozilla.org/?tree=Try&rev=380a095c205d
(Reporter)

Updated

5 years ago
Attachment #635311 - Flags: review?(sworkman)
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+
(Reporter)

Comment 3

5 years ago
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
(Reporter)

Updated

5 years ago
Depends on: 767158
(Reporter)

Updated

5 years ago
Depends on: 767159
(Reporter)

Updated

5 years ago
Whiteboard: [Snappy:P1]
(Reporter)

Comment 4

5 years ago
Created attachment 635478 [details] [diff] [review]
fix v1.1

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
Looks good to me. You can carry over the r+.
(Reporter)

Comment 6

5 years ago
I think there is also synchronous DNS resolution in this test:

http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsProxyAutoConfig.js
That's not a test, that's PAC... and that's hard to fix
(Reporter)

Updated

5 years ago
Depends on: 769764
(Reporter)

Updated

5 years ago
Depends on: 769782
(Reporter)

Comment 8

5 years ago
Created attachment 637990 [details] [diff] [review]
fix v1.2

Update API documentation.
Attachment #635478 - Attachment is obsolete: true
(Reporter)

Comment 9

5 years ago
Created attachment 662140 [details] [diff] [review]
fix v1.3

https://tbpl.mozilla.org/?tree=Try&rev=331060187a57
Attachment #637990 - Attachment is obsolete: true
(Reporter)

Comment 10

5 years ago
Well, that try run is going to fail since Patrick's proxy patches got backed out.
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

5 years ago
Created attachment 665900 [details] [diff] [review]
fix v1.4

https://tbpl.mozilla.org/?tree=Try&rev=15e4788b3f96
Attachment #662140 - Attachment is obsolete: true
Comment on attachment 665900 [details] [diff] [review]
fix v1.4

Delegating review to Steve.
Attachment #665900 - Flags: review?(sworkman)
(Reporter)

Comment 14

5 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

5 years ago
pushed to mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/02a62c14ec3b
https://hg.mozilla.org/mozilla-central/rev/02a62c14ec3b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 815989
Keywords: addon-compat, dev-doc-needed
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
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

4 years ago
Assigning to Patrick since he is at least following the progress of blocking work more than I am.
Assignee: joshmoz → mcmanus
Assignee: mcmanus → nobody
Whiteboard: [Snappy:P1] → [Snappy:P1][necko-backlog]
You need to log in before you can comment on or make changes to this bug.