Open Bug 766973 Opened 8 years ago Updated 2 years ago

don't allow synchronous DNS queries from the main thread

Categories

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

defect

Tracking

()

REOPENED
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)

Attached patch fix v1.0 (obsolete) — 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.
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+
Depends on: 767158
Depends on: 767159
Whiteboard: [Snappy:P1]
Attached patch fix v1.1 (obsolete) — Splinter Review
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+.
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
Depends on: 769764
Depends on: 769782
Attached patch fix v1.2 (obsolete) — Splinter Review
Update API documentation.
Attachment #635478 - Attachment is obsolete: true
Attached patch fix v1.3 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=331060187a57
Attachment #637990 - Attachment is obsolete: true
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?
Attached patch fix v1.4Splinter Review
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)
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)
https://hg.mozilla.org/mozilla-central/rev/02a62c14ec3b
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 815989
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 → ---
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]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P1
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: P1 → P3
You need to log in before you can comment on or make changes to this bug.