459724 - DNS: page reload should set bypass cache flag

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Patrick McManus [:mcmanus]]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.3) Gecko/2008092515 Ubuntu/8.10 (intrepid) Firefox/3.0.1
Build Identifier: 

this is a follow on bug to 208312

The DNS IDL defines RESOLVE_BYPASS_CACHE as a flag to bypass the DNS cache, but no code in mozilla-central invokes this.

A manual reload should force the cache entry to be refreshed.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Reed Loden [:reed]]

This bites me all the time, so would love a fix for this! :)

Comment 2

[Reed Loden [:reed]]

RES_BYPASS_CACHE, not RESOLVE_BYPASS_CACHE, correct?

Comment 3

[Reed Loden [:reed]]

n/m, I see there's both in the code.

Comment 4

[Patrick McManus [:mcmanus]]

just to be clear, this bug addresses bypassing of the mozilla dns cache service. Any caching resolver embedded in the OS in the local lookup chain will still be used.

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: Fix normalization of flags in DNS cache

This patch fixes a problem with the normalization of flags used in the DNS cache. 

While a bypass flag is already implemented in the class and in the DNS service IDL, the C++ implementation does not normalize it correctly. 

This patch is also included as part of the patch attached to 453403 (pre-fetch dns) as the bug gets in the way of any flag usage (and 453403 adds new flags). I'll update that bug as well to break it out there too.

This patch is necessary, but not sufficient, for closing this bug.

Comment 6

[Patrick McManus [:mcmanus]]

Attachment: reload action will bypass dns cache

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 343149 [details] [diff] [review]
reload action will bypass dns cache

>+++ b/netwerk/base/public/nsISocketTransport.idl
>+    // connection flags

Probably document that these can be used to modify the behavior of the underlying socket connection?

>+    attribute unsigned long connectionFlags;
>+    const unsigned long BYPASS_CACHE = (1 << 0);

I'd separate this out with a comment like:

  // Values for connectionFlags

and then document what BYPASS_CACHE actually does if we can do that usefully (what cache does it bypass, exactly?).

>+++ b/netwerk/base/src/nsSocketTransport2.cpp
>@@ -759,17 +760,17 @@ nsSocketTransport::Init(const char **typ
>-
>+    

Please lose the whitespace change.

>+    rv = dns->AsyncResolve(SocketHost(), (mConnectionFlags & nsSocketTransport::BYPASS_CACHE) ? nsIDNSService::RESOLVE_BYPASS_CACHE : 0,

Can we keep this inside 80 chars?  Maybe have a dnsFlags temporary?

With those nits, looks good.  Sorry for the lag on this...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 343148 [details] [diff] [review]
Fix normalization of flags in DNS cache

> HostDB_MatchEntry(PLDHashTable *table,
>-            he->rec->flags == hk->flags &&
>+        RES_KEY_FLAGS (he->rec->flags) == RES_KEY_FLAGS(hk->flags) &&

Fix the indent to match what we used to have here, and looks good.

Comment 9

[Patrick McManus [:mcmanus]]

Attachment: Fix Normalization of flags in DNS Cache (v2) [Checkin: Comment 13]

addresses review comments from comment 8

Comment 10

[Patrick McManus [:mcmanus]]

Attachment: reload action will bypass dns cache (v2)

address review comments from comment 7. Thanks for taking a look.

Comment 11

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

Comment on attachment 344481 [details] [diff] [review]
reload action will bypass dns cache (v2)

+     * BYPASS_CACHE will force the Necko DNS cache entry to be refreshed with
+     * a new call to NSPR

can you document that this has to be set before opening a stream?

Comment 12

[Patrick McManus [:mcmanus]]

Attachment: reload action will bypass dns cache (v3) [Checkin: Comment 13]

update documentation to reflect Christian's comment in comment 11

Comment 13

[Boris Zbarsky [:bzbarsky]]

Pushed changeset 4542793088dc for the flag normalization patch and changeset 39fd2b88d622 for the actual fix.