1337893 - Isolate DNS Cache using Origin Attributes

Status: RESOLVED FIXED

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

Comment 1

[Tim Huang[:timhuang]]

Attachment: Bug 1337893 - Part 1: Updating idl and ipdl files of DNS to make them originAttributes aware.

Review commit: https://reviewboard.mozilla.org/r/114516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114516/

Comment 2

[Tim Huang[:timhuang]]

Attachment: Bug 1337893 - Part 2: Making the DNS cache be aware of originAttributes.

Review commit: https://reviewboard.mozilla.org/r/114518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114518/

Comment 3

[Tim Huang[:timhuang]]

Attachment: Bug 1337893 - Part 3: Updating Necko for DNS changes.

Review commit: https://reviewboard.mozilla.org/r/114520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114520/

Comment 4

[Tim Huang[:timhuang]]

Attachment: Bug 1337893 - Part 4: Updating whole gecko to make all callers of DNS using correct originAttributes.

Review commit: https://reviewboard.mozilla.org/r/114522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114522/

Comment 5

[Tim Huang[:timhuang]]

Attachment: Bug 1337893 - Part 5: Add a test case for DNS with originAttributes.

Review commit: https://reviewboard.mozilla.org/r/114524/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/114524/

Comment 6

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

Comment on attachment 8839946 [details]
Bug 1337893 - Part 1: Updating idl and ipdl files of DNS to make them originAttributes aware.

https://reviewboard.mozilla.org/r/114516/#review116126

::: netwerk/dns/nsIDNSService.idl:49
(Diff revision 1)
>       *        listener's onLookupComplete should be called.  however, if this
>       *        parameter is null, then onLookupComplete will be called on an
>       *        unspecified thread (possibly recursively).
> +     * @param aOriginAttributes
> +     *        the originAttribute for this resolving, the DNS cache will be separated
> +     *        according to this originAttributes. This attribute is optional to avoid

nit: try to keep these lines under 80 chars.

::: netwerk/dns/nsIDNSService.idl:49
(Diff revision 1)
>       *        listener's onLookupComplete should be called.  however, if this
>       *        parameter is null, then onLookupComplete will be called on an
>       *        unspecified thread (possibly recursively).
> +     * @param aOriginAttributes
> +     *        the originAttribute for this resolving, the DNS cache will be separated
> +     *        according to this originAttributes. This attribute is optional to avoid

nit: try to keep these lines under 80 chars.

Comment 7

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

Comment on attachment 8839947 [details]
Bug 1337893 - Part 2: Making the DNS cache be aware of originAttributes.

https://reviewboard.mozilla.org/r/114518/#review116362

::: netwerk/dns/ChildDNSService.cpp:95
(Diff revision 1)
> +                              uint8_t            aArgc,
>                                nsICancelable    **result)
>  {
> -  return AsyncResolveExtended(hostname, flags, EmptyCString(), listener,
> -                              target_, result);
> +  OriginAttributes attrs;
> +
> +  if (aArgc == 1) {

aArgc is the number of optional arguments, right?
Presumably after we deprecate xpcom addons, we can make OA non-optional. We should have a bug for that after we land this.

Comment 8

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

Comment on attachment 8839948 [details]
Bug 1337893 - Part 3: Updating Necko for DNS changes.

https://reviewboard.mozilla.org/r/114520/#review116364

Comment 9

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

Comment on attachment 8839950 [details]
Bug 1337893 - Part 5: Add a test case for DNS with originAttributes.

https://reviewboard.mozilla.org/r/114524/#review116366

Comment 10

[Tim Huang[:timhuang]]

Comment on attachment 8839946 [details]
Bug 1337893 - Part 1: Updating idl and ipdl files of DNS to make them originAttributes aware.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114516/diff/1-2/

Comment 11

[Tim Huang[:timhuang]]

Comment on attachment 8839947 [details]
Bug 1337893 - Part 2: Making the DNS cache be aware of originAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114518/diff/1-2/

Comment 12

[Tim Huang[:timhuang]]

Comment on attachment 8839948 [details]
Bug 1337893 - Part 3: Updating Necko for DNS changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114520/diff/1-2/

Comment 13

[Tim Huang[:timhuang]]

Comment on attachment 8839949 [details]
Bug 1337893 - Part 4: Updating whole gecko to make all callers of DNS using correct originAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114522/diff/1-2/

Comment 14

[Tim Huang[:timhuang]]

Comment on attachment 8839950 [details]
Bug 1337893 - Part 5: Add a test case for DNS with originAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114524/diff/1-2/

Comment 15

[Tim Huang[:timhuang]]

Sorry Patrick and Valentin, I forgot to modify the reviewer's name when I re-submitted a new version of patches, which makes the review board set review flags incorrectly.

Comment 16

[Tim Huang[:timhuang]]

Comment on attachment 8839946 [details]
Bug 1337893 - Part 1: Updating idl and ipdl files of DNS to make them originAttributes aware.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114516/diff/2-3/

Comment 17

[Tim Huang[:timhuang]]

Comment on attachment 8839947 [details]
Bug 1337893 - Part 2: Making the DNS cache be aware of originAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114518/diff/2-3/

Comment 18

[Tim Huang[:timhuang]]

Comment on attachment 8839948 [details]
Bug 1337893 - Part 3: Updating Necko for DNS changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114520/diff/2-3/

Comment 19

[Tim Huang[:timhuang]]

Comment on attachment 8839949 [details]
Bug 1337893 - Part 4: Updating whole gecko to make all callers of DNS using correct originAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114522/diff/2-3/

Comment 20

[Tim Huang[:timhuang]]

Comment on attachment 8839950 [details]
Bug 1337893 - Part 5: Add a test case for DNS with originAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114524/diff/2-3/

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 8839949 [details]
Bug 1337893 - Part 4: Updating whole gecko to make all callers of DNS using correct originAttributes.

https://reviewboard.mozilla.org/r/114522/#review119120

::: dom/html/nsHTMLDNSPrefetch.cpp:143
(Diff revision 3)
>      // considers empty strings to be valid hostnames
>      if (!hostname.IsEmpty() &&
>          net_IsValidHostName(NS_ConvertUTF16toUTF8(hostname))) {
>        // during shutdown gNeckoChild might be null
>        if (gNeckoChild) {
> -        gNeckoChild->SendHTMLDNSPrefetch(nsAutoString(hostname), flags);
> +        gNeckoChild->SendHTMLDNSPrefetch(nsAutoString(hostname),

nsString()

::: dom/html/nsHTMLDNSPrefetch.cpp:192
(Diff revision 3)
>      return NS_ERROR_NOT_AVAILABLE;
>  
>    nsAutoString hostname;
>    aElement->GetHostname(hostname);
> -  return CancelPrefetch(hostname, flags, aReason);
> +  return CancelPrefetch(hostname,
> +                        aElement->GetElement()

check that GetElement() is not null.

::: dom/html/nsHTMLDNSPrefetch.cpp:350
(Diff revision 3)
>          if (!hostName.IsEmpty() && NS_SUCCEEDED(rv) && !isLocalResource) {
>            if (IsNeckoChild()) {
>              // during shutdown gNeckoChild might be null
>              if (gNeckoChild) {
>                gNeckoChild->SendHTMLDNSPrefetch(NS_ConvertUTF8toUTF16(hostName),
> +                                               link->GetElement()

same here.

::: dom/html/nsHTMLDNSPrefetch.cpp:362
(Diff revision 3)
>  
> -            rv = sDNSService->AsyncResolve(hostName,
> +            rv = sDNSService->AsyncResolveNative(hostName,
> -                                           mEntries[mTail].mFlags
> +                                                 mEntries[mTail].mFlags
> -                                           | nsIDNSService::RESOLVE_SPECULATE,
> +                                                 | nsIDNSService::RESOLVE_SPECULATE,
> -                                           sDNSListener, nullptr,
> +                                                 sDNSListener, nullptr,
> +                                                 link->GetElement()

here as well

::: dom/html/nsHTMLDNSPrefetch.cpp:373
(Diff revision 3)
>                link->OnDNSPrefetchRequested();
>            }
>          }
>        }
>      }
>      

just because you are touching this file, do you mind to remove this extra spaces?

Comment 22

[Tim Huang[:timhuang]]

Comment on attachment 8839946 [details]
Bug 1337893 - Part 1: Updating idl and ipdl files of DNS to make them originAttributes aware.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114516/diff/3-4/

Comment 23

[Tim Huang[:timhuang]]

Comment on attachment 8839947 [details]
Bug 1337893 - Part 2: Making the DNS cache be aware of originAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114518/diff/3-4/

Comment 24

[Tim Huang[:timhuang]]

Comment on attachment 8839948 [details]
Bug 1337893 - Part 3: Updating Necko for DNS changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114520/diff/3-4/

Comment 25

[Tim Huang[:timhuang]]

Comment on attachment 8839949 [details]
Bug 1337893 - Part 4: Updating whole gecko to make all callers of DNS using correct originAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114522/diff/3-4/

Comment 26

[Tim Huang[:timhuang]]

Comment on attachment 8839950 [details]
Bug 1337893 - Part 5: Add a test case for DNS with originAttributes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/114524/diff/3-4/

Comment 27

[Tim Huang[:timhuang]]

Try looks good.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d16f4ce25601

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8ee0c21d462
Part 1: Updating idl and ipdl files of DNS to make them originAttributes aware. r=valentin
https://hg.mozilla.org/integration/autoland/rev/7405ef5589cc
Part 2: Making the DNS cache be aware of originAttributes. r=valentin
https://hg.mozilla.org/integration/autoland/rev/a98c0d18ca5e
Part 3: Updating Necko for DNS changes. r=valentin
https://hg.mozilla.org/integration/autoland/rev/2e4be7d6a0a8
Part 4: Updating whole gecko to make all callers of DNS using correct originAttributes. r=baku
https://hg.mozilla.org/integration/autoland/rev/a339ae7dcfa3
Part 5: Add a test case for DNS with originAttributes. r=valentin

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b8ee0c21d462
https://hg.mozilla.org/mozilla-central/rev/7405ef5589cc
https://hg.mozilla.org/mozilla-central/rev/a98c0d18ca5e
https://hg.mozilla.org/mozilla-central/rev/2e4be7d6a0a8
https://hg.mozilla.org/mozilla-central/rev/a339ae7dcfa3