Last Comment Bug 766973 - don't allow synchronous DNS queries from the main thread
: don't allow synchronous DNS queries from the main thread
Status: REOPENED
[Snappy:P1][necko-backlog]
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: Networking: DNS (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: mozilla19
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 767158 767159 769764 769782 815989
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-21 07:49 PDT by Josh Aas
Modified: 2016-02-16 11:22 PST (History)
18 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1.0 (1000 bytes, patch)
2012-06-21 07:49 PDT, Josh Aas
sjhworkman: review+
Details | Diff | Splinter Review
fix v1.1 (1.05 KB, patch)
2012-06-21 14:42 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.2 (2.24 KB, patch)
2012-06-29 13:23 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.3 (2.25 KB, patch)
2012-09-18 06:56 PDT, Josh Aas
no flags Details | Diff | Splinter Review
fix v1.4 (2.24 KB, patch)
2012-09-28 07:49 PDT, Josh Aas
no flags Details | Diff | Splinter Review

Description Josh Aas 2012-06-21 07:49:01 PDT
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.
Comment 1 Josh Aas 2012-06-21 07:51:18 PDT
try server run

https://tbpl.mozilla.org/?tree=Try&rev=380a095c205d
Comment 2 Steve Workman [:sworkman] (INACTIVE) 2012-06-21 10:56:12 PDT
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 4 Josh Aas 2012-06-21 14:42:38 PDT
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.
Comment 5 Steve Workman [:sworkman] (INACTIVE) 2012-06-21 14:47:58 PDT
Looks good to me. You can carry over the r+.
Comment 6 Josh Aas 2012-06-27 07:27:10 PDT
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) 2012-06-29 12:10:59 PDT
That's not a test, that's PAC... and that's hard to fix
Comment 8 Josh Aas 2012-06-29 13:23:47 PDT
Created attachment 637990 [details] [diff] [review]
fix v1.2

Update API documentation.
Comment 10 Josh Aas 2012-09-18 08:04:12 PDT
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) 2012-09-18 11:39:01 PDT
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 13 Jason Duell [:jduell] (needinfo me) 2012-09-28 16:57:18 PDT
Comment on attachment 665900 [details] [diff] [review]
fix v1.4

Delegating review to Steve.
Comment 14 Josh Aas 2012-09-28 17:00:23 PDT
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 2012-09-28 21:52:02 PDT
pushed to mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/02a62c14ec3b
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-09-29 09:40:22 PDT
https://hg.mozilla.org/mozilla-central/rev/02a62c14ec3b
Comment 17 Patrick McManus [:mcmanus] PTO until Sep 6 2012-12-03 13:10:28 PST
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] PTO until Sep 6 2013-01-22 09:11:09 PST
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 2013-02-06 19:14:07 PST
Assigning to Patrick since he is at least following the progress of blocking work more than I am.

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