Closed Bug 1337893 Opened 6 years ago Closed 6 years ago

Isolate DNS Cache using Origin Attributes

Categories

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

Product:
Core
Component:
Networking: DNS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: tjr, Assigned: timhuang)

References

Details

(Whiteboard: [OA][tor][necko-backlog])

Attachments

(5 files)

Bug 1337893 - Part 1: Updating idl and ipdl files of DNS to make them originAttributes aware.
59 bytes, text/x-review-board-request
Details
Bug 1337893 - Part 2: Making the DNS cache be aware of originAttributes.
59 bytes, text/x-review-board-request
Details
Bug 1337893 - Part 3: Updating Necko for DNS changes.
59 bytes, text/x-review-board-request
Details
Bug 1337893 - Part 4: Updating whole gecko to make all callers of DNS using correct originAttributes.
59 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1337893 - Part 5: Add a test case for DNS with originAttributes.
59 bytes, text/x-review-board-request
Details 
      No description provided.
Priority: -- → P1
Assignee: nobody → tihuang
Whiteboard: [OA][tor] → [OA][tor][necko-backlog]
Attachment #8839946 - Flags: review?(mcmanus) → review?(valentin.gosu)
Attachment #8839947 - Flags: review?(mcmanus) → review?(valentin.gosu)
Attachment #8839948 - Flags: review?(mcmanus) → review?(valentin.gosu)
Attachment #8839950 - Flags: review?(mcmanus) → review?(valentin.gosu)
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.
Attachment #8839946 - Flags: review?(valentin.gosu) → review+
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.
Attachment #8839947 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8839948 [details]
Bug 1337893 - Part 3: Updating Necko for DNS changes.

https://reviewboard.mozilla.org/r/114520/#review116364
Attachment #8839948 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8839950 [details]
Bug 1337893 - Part 5: Add a test case for DNS with originAttributes.

https://reviewboard.mozilla.org/r/114524/#review116366
Attachment #8839950 - Flags: review?(valentin.gosu) → review+
Attachment #8839948 - Flags: review?(mcmanus) → review?(valentin.gosu)
Attachment #8839947 - Flags: review?(mcmanus) → review?(valentin.gosu)
Attachment #8839946 - Flags: review?(mcmanus) → review?(valentin.gosu)
Attachment #8839950 - Flags: review?(mcmanus) → review?(valentin.gosu)
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.
Attachment #8839946 - Flags: review?(valentin.gosu) → review+
Attachment #8839947 - Flags: review?(valentin.gosu) → review+
Attachment #8839948 - Flags: review?(valentin.gosu) → review+
Attachment #8839950 - Flags: review?(valentin.gosu) → review+
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?
Attachment #8839949 - Flags: review?(amarchesini) → review+
Pushed by jhao@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.