766973 - don't allow synchronous DNS queries from the main thread

Status: RESOLVED DUPLICATE of bug 1398646

(Core :: Networking: DNS, defect, P3)

Description

[Josh Aas]

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

Comment 1

[Josh Aas]

try server run

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

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 3

[Josh Aas]

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

Comment 4

[Josh Aas]

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

Comment 5

[Steve Workman [:sworkman] (INACTIVE)]

Looks good to me. You can carry over the r+.

Comment 6

[Josh Aas]

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

[Christian :Biesinger (don't email me, ping me on IRC)]

That's not a test, that's PAC... and that's hard to fix

Comment 8

[Josh Aas]

Attachment: fix v1.2

Update API documentation.

Comment 9

[Josh Aas]

Attachment: fix v1.3

https://tbpl.mozilla.org/?tree=Try&rev=331060187a57

Comment 10

[Josh Aas]

Well, that try run is going to fail since Patrick's proxy patches got backed out.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 12

[Josh Aas]

Attachment: fix v1.4

https://tbpl.mozilla.org/?tree=Try&rev=15e4788b3f96

Comment 13

[Jason Duell]

Comment on attachment 665900 [details] [diff] [review]
fix v1.4

Delegating review to Steve.

Comment 14

[Josh Aas]

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.

Comment 15

[Josh Aas]

pushed to mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/02a62c14ec3b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/02a62c14ec3b

Comment 17

[Patrick McManus [:mcmanus]]

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.

Comment 18

[Patrick McManus [:mcmanus]]

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.

Comment 19

[Josh Aas]

Assigning to Patrick since he is at least following the progress of blocking work more than I am.

Comment 20

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 21

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 22

[Valentin Gosu [:valentin] (he/him)]

This was fixed in bug 1398646.