Making the network predictor obey originAttributes and updating SpeculativeConnect() to SpeculativeConnect2().

RESOLVED FIXED in Firefox 54

Status

()

Core
DOM: Security
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: timhuang, Assigned: timhuang)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [tor] [domsecurity-active][OA])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(24 attachments)

59 bytes, text/x-review-board-request
mayhemer
: review+
Details | Review
59 bytes, text/x-review-board-request
mayhemer
: review+
Details | Review
59 bytes, text/x-review-board-request
mayhemer
: review+
Details | Review
59 bytes, text/x-review-board-request
mayhemer
: review+
Details | Review
59 bytes, text/x-review-board-request
nwgh
: review+
Details | Review
59 bytes, text/x-review-board-request
smaug
: review+
Details | Review
59 bytes, text/x-review-board-request
nwgh
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
59 bytes, text/x-review-board-request
Gijs
: review+
Details | Review
59 bytes, text/x-review-board-request
ckerschb
: review+
Details | Review
59 bytes, text/x-review-board-request
sebastian
: review+
Details | Review
59 bytes, text/x-review-board-request
mayhemer
: review+
Details | Review
10.89 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
5.60 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
5.46 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
3.44 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
51.58 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
7.90 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
9.96 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
1.49 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
6.56 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
1.20 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
3.18 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
1.32 KB, patch
timhuang
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
In Bug 1304219, we added a new interface speculativeConnect2() in nsISpeculativeConnect which takes a nsIPrincipal as an additional argument. The speculativeConnect2() will use this principal, instead of the system principal, to establish a channel for the hinted transaction. The Bug 1304219 has updated a part of Gecko, now, we need to update for whole Gecko.
Ethan: should this be a P1 like the bug it was spun off from (bug 1304219)?
Blocks: 1299996
Flags: needinfo?(ettseng)
Currently SpeculativeConnect() is mainly used in four components in Gecko:
- DOM for rel=preconnect
- Necko for network predictor
- Awesome Bar
- Fennec

In order to fulfill the requirement of FirstPartyIsolation, we have to replace SpeculativeConnect()
by SpeculativeConnect2() in DOM and Necko.  This is what Tim plans to accomplish in bug 1304219.

This bug was filed to track the rest replacements in Gecko, i.e. in Awesome Bar and Fennec.
They should not be urgent works.
Flags: needinfo?(ettseng)
Priority: -- → P3
(In reply to Ethan Tseng [:ethan] from comment #2)
> In order to fulfill the requirement of FirstPartyIsolation, we have to
> replace SpeculativeConnect()
> by SpeculativeConnect2() in DOM and Necko.  This is what Tim plans to
> accomplish in bug 1304219.

Correct my own comment to make it more precise.

After reading comment 13 and 14 in bug 1304219, I realize that Tim's plan it to use SpeculativeConnect2() in network predictor but not really change its current behavior (because predictor doesn't get the right principal yet).
It means predictor will not be separated by bug 1304219, and that work should be done in this bug.

Updated

a year ago
Whiteboard: [tor]

Updated

a year ago
Whiteboard: [tor] → [tor] [domsecurity-backlog1]

Updated

a year ago
Blocks: 1309816
(Assignee)

Updated

a year ago
Assignee: nobody → tihuang
(Assignee)

Comment 4

a year ago
I am going to make the network predictor obeys originAttributes in the bug as Ethan mentioned in comment 3.
Priority: P3 → P1
Summary: Updating SpeculativeConnect() to SpeculativeConnect2(). → Making the network predictor obeys originAttriubtes and updating SpeculativeConnect() to SpeculativeConnect2().
Whiteboard: [tor] [domsecurity-backlog1] → [tor] [domsecurity-active]

Updated

a year ago
Whiteboard: [tor] [domsecurity-active] → [tor] [domsecurity-active][OA]

Updated

a year ago
Blocks: 1191418
(Assignee)

Comment 5

a year ago
I would like to add a new function 'asyncVisitEntireEntries' in nsICacheStorageStorage to visit all cache entries in the disk cache and memory cache which ignores the nsIloadContextInfo. The reason why I do this is because the 'reset' function in nsINetworkPredictor has to visit all entries for clearing all predictor information. However, there is no such way to do this job in the current state.

In addition, I will add a getter for nsILoadContextInfo in nsICacheEntry.idl. This because the network predictor needs this feature to open the correct storage with this loadContextInfo when it visits cache entries.

Updated

a year ago
Summary: Making the network predictor obeys originAttriubtes and updating SpeculativeConnect() to SpeculativeConnect2(). → Making the network predictor obey originAttributes and updating SpeculativeConnect() to SpeculativeConnect2().
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

a year ago
mozreview-review
Comment on attachment 8823643 [details]
Bug 1312954 - Part 6: Updating all callers of network predictor, and the docshell will update the first party domain if it is a typeContent mozbrowser.

https://reviewboard.mozilla.org/r/102212/#review102592

::: docshell/base/nsDocShell.cpp:10686
(Diff revision 2)
>      srcdoc = aSrcdoc;
>    } else {
>      srcdoc = NullString();
>    }
>  
> +  NeckoOriginAttributes neckoAttrs;

Why !GetIsMozBrowser() check? I would expect MozBrowser docshells be treated as if they were top-level

::: docshell/base/nsDocShell.cpp:10687
(Diff revision 2)
>    } else {
>      srcdoc = NullString();
>    }
>  
> +  NeckoOriginAttributes neckoAttrs;
> +  bool isTopLevelDoc = isTargetTopLevelDocShell && mItemType == typeContent &&

This seems to be wrong.
isTargetTopLevelDocShell isn't always set when doing top level load. It is for the case when target is explicitly defined.

Perhaps you could set isTargetTopLevelDocShell true in the 'else' of 'if (IsFrame() && !isTargetTopLevelDocShell) {' above
Attachment #8823643 - Flags: review?(bugs) → review-

Comment 19

a year ago
mozreview-review
Comment on attachment 8823643 [details]
Bug 1312954 - Part 6: Updating all callers of network predictor, and the docshell will update the first party domain if it is a typeContent mozbrowser.

https://reviewboard.mozilla.org/r/102212/#review102606

::: docshell/base/nsDocShell.cpp:10686
(Diff revision 2)
>      srcdoc = aSrcdoc;
>    } else {
>      srcdoc = NullString();
>    }
>  
> +  NeckoOriginAttributes neckoAttrs;

Oh, I see you took GetIsMozBrowser check from the other a bit similar case where firstPartyDomain stuff is being handled. But I think that code is actually wrong.

(I was wondering this stuff also in bug 1260931 comment 85, but apparently that comment wasn't ever properly addressed.)
Comment on attachment 8823642 [details]
Bug 1312954 - Part 5: Have the network predictor use OriginAttributes to properly partition connections it creates like non-predictive connections.

https://reviewboard.mozilla.org/r/102210/#review102602

This all looks pretty good, once the bits below are fixed. One other thing - the commit message isn't as descriptive as I would like. Maybe something like the following would be more appropriate:

"Have the network predictor use OriginAttributes to properly partition connections it creates like non-predictive connections."

Just a thought.

::: netwerk/base/Predictor.cpp:787
(Diff revision 2)
> +
> +  return PredictNative(targetURI, sourceURI, reason, attrs, verifier);
> +}
> +
> +// Called from the main thread to initiate predictive actions
> +NS_IMETHODIMP_(nsresult)

Please just use NS_IMETHODIMP here (and other places you've used this form). It literally expands to what you've typed here, and is consistent with the rest of the file.

::: netwerk/base/Predictor.cpp:791
(Diff revision 2)
> +// Called from the main thread to initiate predictive actions
> +NS_IMETHODIMP_(nsresult)
> +Predictor::PredictNative(nsIURI *targetURI, nsIURI *sourceURI,
> +                         PredictorPredictReason reason,
> +                         const NeckoOriginAttributes& originAttributes,
> -                   nsINetworkPredictorVerifier *verifier)
> +                         nsINetworkPredictorVerifier *verifier)

nit: reviewboard indicates a tab got put here, please ensure spaces for indentation.

::: netwerk/base/Predictor.cpp:930
(Diff revision 2)
>                   reason, originAction.get()));
>    openFlags = nsICacheStorage::OPEN_READONLY |
>                nsICacheStorage::OPEN_SECRETLY |
>                nsICacheStorage::CHECK_MULTITHREADED;
> -  mCacheDiskStorage->AsyncOpenURI(originKey,
> +  cacheDiskStorage->AsyncOpenURI(originKey,
>                                    NS_LITERAL_CSTRING(PREDICTOR_ORIGIN_EXTENSION),

nit: please fix indentation here to match the change in the previous line (and other places, I'm not going to drive both myself and you crazy commenting on every single one).

::: netwerk/base/Predictor.cpp:1617
(Diff revision 2)
> +
> +  RefPtr<LoadContextInfo> lci =
> +    new LoadContextInfo(false, originAttributes);
> +
> +  rv = mCacheStorageService->DiskCacheStorage(lci, false,
> +                                             getter_AddRefs(cacheDiskStorage));

nit: looks like "getter_AddRefs" should be indented one more space. May be something wrong with the initial indentation, but worth fixing since we're here. Also look at other instances of this, just in case.

::: netwerk/base/Predictor.cpp:2239
(Diff revision 2)
>    nsCOMPtr<nsINetworkPredictor> predictor;
>    nsresult rv = EnsureGlobalPredictor(getter_AddRefs(predictor));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  return predictor->Predict(targetURI, sourceURI, reason,
> -                            loadContext, verifier);
> +  return predictor->PredictNative(targetURI, sourceURI, reason,
> +                            originAttributes, verifier);

nit: (since this is a new-ish version of this issue) again, please fix indentation to match with opening parens.

::: netwerk/base/Predictor.cpp:2319
(Diff revision 2)
> -    loadContext = document->GetLoadContext();
> +    nsCOMPtr<nsIPrincipal> docPrincipal = document->NodePrincipal();
> +
> +    if (docPrincipal) {
> +      PrincipalOriginAttributes docAttrs = docPrincipal->OriginAttributesRef();
> +      originAttributes.InheritFromDocToNecko(docAttrs);
> -  }
> +    }

nit: tab

::: netwerk/base/Predictor.cpp:2321
(Diff revision 2)
> +      PrincipalOriginAttributes docAttrs = docPrincipal->OriginAttributesRef();
> +      originAttributes.InheritFromDocToNecko(docAttrs);
> -  }
> +    }
>  
> -  return predictor->Learn(targetURI, sourceURI, reason, loadContext);
> -}
> +  }

nit: tab

::: netwerk/base/Predictor.cpp:2353
(Diff revision 2)
>    nsCOMPtr<nsINetworkPredictor> predictor;
>    rv = EnsureGlobalPredictor(getter_AddRefs(predictor));
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  return predictor->Learn(targetURI, sourceURI,
> +  return predictor->LearnNative(targetURI, sourceURI,
> -                          nsINetworkPredictor::LEARN_LOAD_REDIRECT,
> +                                nsINetworkPredictor::LEARN_LOAD_REDIRECT,

nit: tab

::: netwerk/base/nsINetworkPredictor.idl:80
(Diff revision 2)
> +  [notxpcom]
> +  nsresult predictNative(in nsIURI targetURI,
> +                         in nsIURI sourceURI,
> +                         in PredictorPredictReason reason,
> +                         in NeckoOriginAttributes originAttributes,
> -               in nsINetworkPredictorVerifier verifier);
> +                         in nsINetworkPredictorVerifier verifier);

nit: tab got inserted.
Attachment #8823642 - Flags: review?(hurley) → review+
Comment on attachment 8823644 [details]
Bug 1312954 - Part 7: Updating the tests of network predictor.

https://reviewboard.mozilla.org/r/102214/#review102654

This looks fine, just one change to match the file's style and it's good to go.

::: netwerk/test/unit/test_predictor.js:27
(Diff revision 2)
>      o = o + ":" + uri.port;
>    }
>    return o;
>  }
>  
> -var LoadContext = function _loadContext() {
> +var originAttributes = {};

I know the style of this file doesn't match our usual camelCase naming, but please have this match the file's style (so origin_attributes instead of originAttributes).
Attachment #8823644 - Flags: review?(hurley) → review+
(Assignee)

Updated

a year ago
Attachment #8823638 - Flags: review?(honzab.moz)
Attachment #8823639 - Flags: review?(honzab.moz)
Attachment #8823640 - Flags: review?(honzab.moz)
Attachment #8823641 - Flags: review?(honzab.moz)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 25

a year ago
mozreview-review-reply
Comment on attachment 8823643 [details]
Bug 1312954 - Part 6: Updating all callers of network predictor, and the docshell will update the first party domain if it is a typeContent mozbrowser.

https://reviewboard.mozilla.org/r/102212/#review102606

> Oh, I see you took GetIsMozBrowser check from the other a bit similar case where firstPartyDomain stuff is being handled. But I think that code is actually wrong.
> 
> (I was wondering this stuff also in bug 1260931 comment 85, but apparently that comment wasn't ever properly addressed.)

Thanks, smaug. I have updated this part and other places for first party isolation.

Comment 26

a year ago
mozreview-review
Comment on attachment 8823643 [details]
Bug 1312954 - Part 6: Updating all callers of network predictor, and the docshell will update the first party domain if it is a typeContent mozbrowser.

https://reviewboard.mozilla.org/r/102212/#review103432

::: docshell/base/nsDocShell.cpp:3602
(Diff revision 3)
> -        aTargetItem->ItemType() == nsIDocShellTreeItem::typeContent &&
> -        !targetDS->GetIsMozBrowser()) {
> +
> +    if (targetDS->GetIsMozBrowser()) {
> +      nsCOMPtr<nsIDocShellTreeItem> parent;
> +      aTargetItem->GetParent(getter_AddRefs(parent));
> +      if (parent) {
> +        isTopLevelDocShell = parent->ItemType() == nsIDocShellTreeItem::typeContent &&

I don't understand this check. Why parent needs to be the same type if we're typeContent mozbrowser?

::: docshell/base/nsDocShell.cpp:10714
(Diff revision 3)
>    }
>  
> +  NeckoOriginAttributes neckoAttrs;
> +  bool isTopLevelDoc = false;
> +  if (GetIsMozBrowser()) {
> +    // We consider a MozBrowser as top-level when its parent is also

Why the parent needs to be content? Shouldn't any top level content docshell, and any typeContent mozbrowser be considered as top level

::: docshell/base/nsDocShell.cpp:10964
(Diff revision 3)
>    // OriginAttributes of the parent document. Or in case there isn't a
>    // parent document.
>    NeckoOriginAttributes neckoAttrs;
> -  bool isTopLevelDoc = aContentPolicyType == nsIContentPolicy::TYPE_DOCUMENT &&
> -                       mItemType == typeContent &&
> -                       !GetIsMozBrowser();
> +  bool isTopLevelDoc = false;
> +  if (GetIsMozBrowser()) {
> +    // We consider a MozBrowser as top-level when its parent is also

And here
Attachment #8823643 - Flags: review?(bugs) → review-
(Assignee)

Comment 27

a year ago
(In reply to Olli Pettay [:smaug] from comment #26)
> Comment on attachment 8823643 [details]
> Bug 1312954 - Part 6: Updating all callers of network predictor, and making
> the mozbrowser updates its first party domain if its parent is also
> typeContent.
> 
> https://reviewboard.mozilla.org/r/102212/#review103432
> 

I misunderstood the bug 1260931 comment 85. I will submit another patch with a proper fix. Thanks.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 30

a year ago
mozreview-review-reply
Comment on attachment 8823642 [details]
Bug 1312954 - Part 5: Have the network predictor use OriginAttributes to properly partition connections it creates like non-predictive connections.

https://reviewboard.mozilla.org/r/102210/#review102602

> nit: reviewboard indicates a tab got put here, please ensure spaces for indentation.

I am sure about that they are spaces. I don't know why the reviewborad indicates a tab here.

> nit: tab

Same here

> nit: tab

Same here

> nit: tab

Same here

> nit: tab got inserted.

Same here

Comment 31

a year ago
mozreview-review
Comment on attachment 8823643 [details]
Bug 1312954 - Part 6: Updating all callers of network predictor, and the docshell will update the first party domain if it is a typeContent mozbrowser.

https://reviewboard.mozilla.org/r/102212/#review104064
Attachment #8823643 - Flags: review?(bugs) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8826444 [details]
Bug 1312954 - Part 8: Updating speculativeConnect to speculativeConnect2 for about:newtab.

Forwarding to an active browser peer. Please forward again if necessary :)
Attachment #8826444 - Flags: review?(ttaubert) → review?(gijskruitbosch+bugs)

Comment 42

a year ago
mozreview-review
Comment on attachment 8826444 [details]
Bug 1312954 - Part 8: Updating speculativeConnect to speculativeConnect2 for about:newtab.

https://reviewboard.mozilla.org/r/104400/#review105216

::: browser/base/content/newtab/sites.js:319
(Diff revision 1)
> +      let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
> +      sc.speculativeConnect2(uri, systemPrincipal, null);

I have two concerns that make me r- and suggest you ask :ckerschb.

1) using system principal is a red flag. Is there something else we can use that makes more sense?
2) the summary of this bug mentions originAttributes and using the right ones for speculative connections. But the system principal has no usable originAttributes. More precisely, if you load about:newtab in a container tab, it will speculatively connect independent of the container tab, which (again, based on what little I understand of how speculative connect works) means we can't reuse the connection, AIUI (given that you'll need e.g. cookies which might be in another cookie jar)

I suspect the right thing is to:

- ignore non-http/https URIs.
- create a principal off the URI's *own* codebase and a correct set of origin attributes (ie with the right userContextId).

But I defer to :baku and/or :ckerschb .
Attachment #8826444 - Flags: review?(gijskruitbosch+bugs) → review-

Comment 43

a year ago
mozreview-review
Comment on attachment 8826445 [details]
Bug 1312954 - Part 9: Updating speculativeConnect to speculativeConnect2 for the search service.

https://reviewboard.mozilla.org/r/104402/#review105218

r- here for the same reason - `window` is a chrome window, and so its nodePrincipal will be system, so it will have no sensible origin attributes. We should use the contentPrincipal of the current browser instead.


Ideally there should be tests for this stuff. Do we have some way we can assert that the right speculative connections are being made?
Attachment #8826445 - Flags: review-
(Assignee)

Comment 44

a year ago
(In reply to :Gijs from comment #42)
> Comment on attachment 8826444 [details]
> Bug 1312954 - Part 8: Updating speculativeConnect to speculativeConnect2 for
> about:newtab.
> 
> https://reviewboard.mozilla.org/r/104400/#review105216
> 
> ::: browser/base/content/newtab/sites.js:319
> (Diff revision 1)
> > + let systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
> > + sc.speculativeConnect2(uri, systemPrincipal, null);
> 
> I have two concerns that make me r- and suggest you ask :ckerschb.
> 
> 1) using system principal is a red flag. Is there something else we can use
> that makes more sense?
> 2) the summary of this bug mentions originAttributes and using the right
> ones for speculative connections. But the system principal has no usable
> originAttributes. More precisely, if you load about:newtab in a container
> tab, it will speculatively connect independent of the container tab, which
> (again, based on what little I understand of how speculative connect works)
> means we can't reuse the connection, AIUI (given that you'll need e.g.
> cookies which might be in another cookie jar)
> 
> I suspect the right thing is to:
> 
> - ignore non-http/https URIs.
> - create a principal off the URI's *own* codebase and a correct set of
> origin attributes (ie with the right userContextId).
> 
> But I defer to :baku and/or :ckerschb .

Gijs, thanks to your advice. The patch I did was to follow the original behavior. However, I think you're right, that we should not use the system principal here, but using a principal from the URI with correct originAttributes.

ni :baku and :ckerschb to see do they have any option regarding this.
Flags: needinfo?(ckerschb)
Flags: needinfo?(amarchesini)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 47

a year ago
mozreview-review
Comment on attachment 8826444 [details]
Bug 1312954 - Part 8: Updating speculativeConnect to speculativeConnect2 for about:newtab.

https://reviewboard.mozilla.org/r/104400/#review105680

::: browser/base/content/newtab/sites.js:324
(Diff revision 2)
> +      let originAttributes = this._node.ownerDocument
> +                                       .defaultView
> +                                       .QueryInterface(Ci.nsIInterfaceRequestor)
> +                                       .getInterface(Ci.nsIDocShell)
> +                                       .getOriginAttributes();

You can replace this with:

```js
let originAttributes = document.docShell.getOriginAttributes();
```
which is a lot more readable. :-)
Attachment #8826444 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 48

a year ago
mozreview-review
Comment on attachment 8823638 [details]
Bug 1312954 - Part 1: Add the nsILoadContextInfo as one additional argument of nsILoadContextInfo.onCacheEntryInfo() and CacheStorageService::EntryInfoCallback.OnEntryInfo().

https://reviewboard.mozilla.org/r/102202/#review105708
Attachment #8823638 - Flags: review?(honzab.moz) → review+

Comment 49

a year ago
mozreview-review
Comment on attachment 8823639 [details]
Bug 1312954 - part 2: Add a new interface nsICacheStorageService.asyncVisitAllStorages() to allow visiting every cache entry.

https://reviewboard.mozilla.org/r/102204/#review105704

::: netwerk/cache2/CacheStorageService.cpp:941
(Diff revision 2)
> +  bool aVisitEntries)
> +{
> +  LOG(("CacheStorageService::AsyncVisitAllStorages [cb=%p]", aVisitor));
> +  NS_ENSURE_FALSE(mShutdown, NS_ERROR_NOT_INITIALIZED);
> +
> +  // Walks the disk cache also walks the memory cache.

"WalkING the disk cache...." ?

::: netwerk/cache2/nsICacheStorageService.idl:115
(Diff revision 2)
>     *    NOTE: the observer MUST implement nsISupportsWeakReference.
>     */
>    void asyncGetDiskConsumption(in nsICacheStorageConsumptionObserver aObserver);
> +
> +  /**
> +   * Asynchronously visits all storages of the disk cache and memeory cache.

nit: typo

::: netwerk/cache2/nsICacheStorageService.idl:123
(Diff revision 2)
> +   *   A visitor callback.
> +   * @param aVisitEntries
> +   *   A boolean indicates whether visits entries.
> +   */
> +  void asyncVisitAllStorages(in nsICacheStorageVisitor aVisitor,
> +                            in boolean aVisitEntries);

nit: indent
Attachment #8823639 - Flags: review?(honzab.moz) → review+

Comment 50

a year ago
mozreview-review
Comment on attachment 8823640 [details]
Bug 1312954 - Part 3: Add a test for nsICacheStorageService.asyncVisitAllStorages()

https://reviewboard.mozilla.org/r/102206/#review105710

r+ with comments addressed on this one

::: netwerk/test/unit/test_cache2-31-visit-all.js:17
(Diff revision 3)
> +    return;
> +  }
> +
> +  var mc = new MultipleCallbacks(8, function() {
> +    do_execute_soon(function() {
> +      syncWithCacheIOThread(function() {

I don't think you ever more need to do syncWithCacheIOThread calls.  or please comment why you need it/want it here

::: netwerk/test/unit/test_cache2-31-visit-all.js:26
(Diff revision 3)
> +                       "http://a/", "http://b/",   // user Context 2
> +                       "http://a/", "http://b/",]; // user Context 3
> +
> +        get_cache_service().asyncVisitAllStorages(
> +          // Test should store 8 entries across 4 originAttributes
> +          new VisitCallback(8, expectedConsumption, entries, function() {

I think you should update the VisitCallback logic to also check for expected loadinfo.

isn't it the whole purpose of this enterprise?
Attachment #8823640 - Flags: review?(honzab.moz) → review+

Comment 51

a year ago
mozreview-review
Comment on attachment 8823641 [details]
Bug 1312954 - Part 4: Add a new functoin in nsICacheEntry to allow its loadInfoContext can be fetched.

https://reviewboard.mozilla.org/r/102208/#review105706

r+ with comments addressed as well

::: netwerk/cache2/CacheEntry.cpp:1563
(Diff revision 3)
> +  nsAutoCString hashingKey;
> +
> +  rv = HashingKeyWithStorage(hashingKey);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsILoadContextInfo> info = CacheFileUtils::ParseKey(hashingKey);

you can just parse the mStorageID member string
Attachment #8823641 - Flags: review?(honzab.moz) → review+

Comment 52

a year ago
mozreview-review
Comment on attachment 8826444 [details]
Bug 1312954 - Part 8: Updating speculativeConnect to speculativeConnect2 for about:newtab.

https://reviewboard.mozilla.org/r/104400/#review105734

::: browser/base/content/newtab/sites.js:331
(Diff revision 2)
> +                                       .QueryInterface(Ci.nsIInterfaceRequestor)
> +                                       .getInterface(Ci.nsIDocShell)
> +                                       .getOriginAttributes();
> +      let prinicpal = Services.scriptSecurityManager
> +                              .createCodebasePrincipal(uri, originAttributes);
> +      sc.speculativeConnect2(uri, prinicpal, null);

typo: principal.
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini [:baku] from comment #52)
> Comment on attachment 8826444 [details]
> Bug 1312954 - Part 8: Updating speculativeConnect to speculativeConnect2 for
> about:newtab.
> 
> https://reviewboard.mozilla.org/r/104400/#review105734
> 
> ::: browser/base/content/newtab/sites.js:331
> (Diff revision 2)
> > +                                       .QueryInterface(Ci.nsIInterfaceRequestor)
> > +                                       .getInterface(Ci.nsIDocShell)
> > +                                       .getOriginAttributes();
> > +      let prinicpal = Services.scriptSecurityManager
> > +                              .createCodebasePrincipal(uri, originAttributes);
> > +      sc.speculativeConnect2(uri, prinicpal, null);
> 
> typo: principal.

Using a codeBasePrincipal is fine and definitely better then falling back on the systemPrincipal - thanks for fixing.
Flags: needinfo?(ckerschb)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 64

a year ago
Your trypush is failing the assertions you're adding, cf. (docshell/domwindow spam snipped):

[task 2017-01-19T11:02:06.647547Z] 11:02:06     INFO - TEST-START | docshell/test/chrome/test_bug89419.xul
[task 2017-01-19T11:02:07.013770Z] 11:02:07     INFO - TEST: registering a listener for pageshow events
[task 2017-01-19T11:02:07.271682Z] 11:02:07     INFO - TEST: eventListener received a pageshow event for page Bug 89419, persisted=false
[task 2017-01-19T11:02:07.288317Z] 11:02:07     INFO - TEST: removing event listeners
[task 2017-01-19T11:02:07.288448Z] 11:02:07     INFO - TEST: registering a listener for pageshow events
[task 2017-01-19T11:02:07.309114Z] 11:02:07     INFO - TEST: Called dummy unload function to prevent page from being bfcached.
[task 2017-01-19T11:02:07.344698Z] 11:02:07     INFO - TEST: eventListener received a pageshow event for page , persisted=false
[task 2017-01-19T11:02:07.360716Z] 11:02:07     INFO - TEST: removing event listeners
[task 2017-01-19T11:02:07.397379Z] 11:02:07     INFO - TEST: registering a listener for pageshow events
[task 2017-01-19T11:02:07.399029Z] 11:02:07     INFO - [3270] ###!!! ASSERTION: We expect passing a principal here.: '!aPrincipal', file /home/worker/workspace/build/src/netwerk/base/nsIOService.cpp, line 1804
[task 2017-01-19T11:02:37.347924Z] 11:02:37     INFO - #01: mozilla::net::Predictor::RunPredictions [netwerk/base/Predictor.cpp:1429]
[task 2017-01-19T11:02:37.354712Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.354805Z] 11:02:37     INFO - #02: mozilla::net::Predictor::PredictForPageload [netwerk/base/Predictor.cpp:1046]
[task 2017-01-19T11:02:37.354866Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.355346Z] 11:02:37     INFO - #03: mozilla::net::Predictor::PredictInternal [netwerk/base/Predictor.cpp:969]
[task 2017-01-19T11:02:37.355421Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.355487Z] 11:02:37     INFO - #04: mozilla::net::Predictor::Action::OnCacheEntryAvailable [netwerk/base/Predictor.cpp:302]
[task 2017-01-19T11:02:37.355524Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.356079Z] 11:02:37     INFO - #05: mozilla::net::CacheEntry::InvokeAvailableCallback [netwerk/cache2/CacheEntry.cpp:864]
[task 2017-01-19T11:02:37.356152Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.356214Z] 11:02:37     INFO - #06: mozilla::net::CacheEntry::InvokeCallback [netwerk/cache2/CacheEntry.cpp:808]
[task 2017-01-19T11:02:37.356251Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.356708Z] 11:02:37     INFO - #07: mozilla::net::CacheEntry::InvokeCallbacks [netwerk/cache2/CacheEntry.cpp:675]
[task 2017-01-19T11:02:37.356779Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.356842Z] 11:02:37     INFO - #08: mozilla::net::CacheEntry::InvokeCallbacks [netwerk/cache2/CacheEntry.cpp:619]
[task 2017-01-19T11:02:37.356879Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.356932Z] 11:02:37     INFO - #09: mozilla::net::CacheEntry::Open [netwerk/cache2/CacheEntry.cpp:339]
[task 2017-01-19T11:02:37.357700Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.357785Z] 11:02:37     INFO - #10: mozilla::net::CacheEntry::AsyncOpen [netwerk/cache2/CacheEntry.cpp:311]
[task 2017-01-19T11:02:37.357824Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.357882Z] 11:02:37     INFO - #11: mozilla::net::CacheStorage::AsyncOpenURI [netwerk/cache2/CacheStorage.cpp:104]
[task 2017-01-19T11:02:37.358398Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.358486Z] 11:02:37     INFO - #12: mozilla::net::Predictor::PredictNative [xpcom/glue/nsCOMPtr.h:1240]
[task 2017-01-19T11:02:37.358531Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.359427Z] 11:02:37     INFO - #13: mozilla::net::PredictorPredict [netwerk/base/Predictor.cpp:2234]
[task 2017-01-19T11:02:37.360263Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.361190Z] 11:02:37     INFO - #14: nsDocShell::InternalLoad [xpcom/glue/nsCOMPtr.h:1195]
[task 2017-01-19T11:02:37.362006Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.362928Z] 11:02:37     INFO - #15: nsDocShell::LoadHistoryEntry [docshell/base/nsDocShell.cpp:12519]
[task 2017-01-19T11:02:37.363753Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.364674Z] 11:02:37     INFO - #16: nsDocShell::LoadURI [docshell/base/nsDocShell.cpp:1434]
[task 2017-01-19T11:02:37.365512Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.366429Z] 11:02:37     INFO - #17: nsSHistory::InitiateLoad [docshell/shistory/nsSHistory.cpp:1899]
[task 2017-01-19T11:02:37.367259Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.368231Z] 11:02:37     INFO - #18: nsSHistory::LoadDifferingEntries [docshell/shistory/nsSHistory.cpp:1796]
[task 2017-01-19T11:02:37.369067Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.369993Z] 11:02:37     INFO - #19: nsSHistory::LoadEntry [docshell/shistory/nsSHistory.cpp:1767]
[task 2017-01-19T11:02:37.370825Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.371773Z] 11:02:37     INFO - #20: nsSHistory::GoBack [docshell/shistory/nsSHistory.cpp:960]
[task 2017-01-19T11:02:37.372594Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.373518Z] 11:02:37     INFO - #21: nsDocShell::GoBack [docshell/base/nsDocShell.cpp:4656]
[task 2017-01-19T11:02:37.374346Z] 11:02:37     INFO - 
[task 2017-01-19T11:02:37.375262Z] 11:02:37     INFO - #22: NS_InvokeByIndex
[task 2017-01-19T11:02:37.376063Z] 11:02:37     INFO -

Comment 65

a year ago
mozreview-review
Comment on attachment 8826445 [details]
Bug 1312954 - Part 9: Updating speculativeConnect to speculativeConnect2 for the search service.

https://reviewboard.mozilla.org/r/104402/#review106650

Better, but I think this still doesn't quite work. Sorry!

::: toolkit/components/search/nsSearchService.js:2581
(Diff revision 3)
>                             .getInterface(Components.interfaces.nsIWebNavigation)
>                             .QueryInterface(Components.interfaces.nsILoadContext);
> +    let principal;
>  
> -    connector.speculativeConnect(searchURI, callbacks);
> +    if (options.window instanceof Ci.nsIDOMChromeWindow) {
> +      principal = options.window.gBrowser.contentPrincipal;

This is toolkit code so you can't assume gBrowser and gBrowser.contentPrincipal exist, plus not all of these chrome windows are necessarily going to be browser windows, even when used in Firefox.

Looking at this more, I'm also wondering what the impact is of having these requests added to chrome windows' load contexts (via the callbacks argument). Is that... safe/sensible? Anyway, that'd be a separate bug.

I would add an option for the principal to the options object and set it explicitly for the search.xml callsite. If no principal is passed, use the stuff in the 'else' clause you wrote (ie ask the docShell for origin attributes and use the searchURI to construct a codebase principal). That will fall back to using the chrome docshell's OA (ie default) for add-ons, which is probably fine as it seems unlikely we end up shipping containers before XUL add-ons are gone (and there aren't a lot of public consumers (anymore), judging by add-ons DXR and spot-checking if some of them were published on AMO).

For the search.xml case, I'm unsure if passing the main browser's contentPrincipal is sensible. In theory, if we have a page with a null principal loaded in the browser's toplevel content (not regularly done by the web, I think, but possible, I think?), that will wreck the speculativeconnect for the browser UI. That seems wrong.

So thinking about it more, perhaps the extra argument on the options object should be the origin attributes, rather than the entire principal, and if none are passed we can fall back on those on the window's docShell. We should always use a codebase principal for the search URI, constructed with those originAttributes.

Does that sound sensible?

::: toolkit/components/search/nsSearchService.js:2583
(Diff revision 3)
> +      // This is for cotent who is using nsSearchService, like the about:newtab.
> +      // We will create a codebase prinicpal from the URI of the search engine
> +      // and the originAttributes from the docShell.

English nits: spelling of 'content', 'principal', and "about:newtab" doesn't need "the" in front of it
Attachment #8826445 - Flags: review?(gijskruitbosch+bugs)

Comment 66

a year ago
mozreview-review
Comment on attachment 8828267 [details]
Bug 1312954 - Part 10: Add an assertion to make sure every code path of gecko uses speculativeConnect2 correctly.

https://reviewboard.mozilla.org/r/105744/#review106714

::: netwerk/base/nsIOService.cpp:1804
(Diff revision 1)
>              do_GetService(NS_PROTOCOLPROXYSERVICE_CONTRACTID, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      nsCOMPtr<nsIPrincipal> loadingPrincipal = aPrincipal;
>  
> +    NS_ASSERTION(!aPrincipal, "We expect passing a principal here.");

Don't we want the opposite assertion here? We wanna make sure that *all* gecko callers pass a principal, right? So it should be:

MOZ_ASSERT(aPrincipal, "...");
Attachment #8828267 - Flags: review?(ckerschb)
Comment hidden (mozreview-request)
(Assignee)

Comment 68

a year ago
mozreview-review-reply
Comment on attachment 8828267 [details]
Bug 1312954 - Part 10: Add an assertion to make sure every code path of gecko uses speculativeConnect2 correctly.

https://reviewboard.mozilla.org/r/105744/#review106714

> Don't we want the opposite assertion here? We wanna make sure that *all* gecko callers pass a principal, right? So it should be:
> 
> MOZ_ASSERT(aPrincipal, "...");

Yes, you are right. Sorry about that I submitted this patch so recklessly. I submitted another patch to fix this.

Comment 69

a year ago
mozreview-review
Comment on attachment 8828267 [details]
Bug 1312954 - Part 10: Add an assertion to make sure every code path of gecko uses speculativeConnect2 correctly.

https://reviewboard.mozilla.org/r/105744/#review106764

Ideally we would use the variable loadingPrincipal from the point we assign aPrincipal to loadingPrincipal, but I am fine with the change itself. thanks
Attachment #8828267 - Flags: review?(ckerschb) → review+
(Assignee)

Comment 70

a year ago
> 
> So thinking about it more, perhaps the extra argument on the options object
> should be the origin attributes, rather than the entire principal, and if
> none are passed we can fall back on those on the window's docShell. We
> should always use a codebase principal for the search URI, constructed with
> those originAttributes.
> 
> Does that sound sensible?
> 

This sounds good to me. Using the codebase principal for the search URI is appropriate here.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 74

a year ago
mozreview-review
Comment on attachment 8826445 [details]
Bug 1312954 - Part 9: Updating speculativeConnect to speculativeConnect2 for the search service.

https://reviewboard.mozilla.org/r/104402/#review106996

::: browser/components/search/content/search.xml:498
(Diff revision 4)
>  
>        <handler event="focus">
>        <![CDATA[
>          // Speculatively connect to the current engine's search URI (and
>          // suggest URI, if different) to reduce request latency
> -        this.currentEngine.speculativeConnect({window});
> +        this.currentEngine.speculativeConnect({ window: window,

You don't need to repeat the `window` parameter, so just:

```
{window,
```

will do.
Attachment #8826445 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

a year ago
Attachment #8828722 - Flags: review?(s.kaspari)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 78

a year ago
mozreview-review
Comment on attachment 8828722 [details]
Bug 1312954 - Part 11: Updating speculativeConnect to speculativeConnect2 for Fennec.

https://reviewboard.mozilla.org/r/106024/#review107936
Attachment #8828722 - Flags: review?(s.kaspari) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 91

a year ago
mozreview-review
Comment on attachment 8830205 [details]
Bug1312954 - Part 12: Updating speculativeConnect to speculativeConnect2 for test_speculative_connect.js.

https://reviewboard.mozilla.org/r/107098/#review109336
Attachment #8830205 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 92

a year ago
Try looks good.
Status: NEW → ASSIGNED
Keywords: checkin-needed
seems some patches need final r+ to be able to use autolander, can you take a look at this bug/patches to make sure we can autoland thanks!
Flags: needinfo?(tihuang)
(Assignee)

Comment 94

a year ago
Honza, it looks like that your r+ doesn't give a final r+. I think It could be that your mozreview account doesn't associate with your LDAP account, see Bug 1305592 comment 21. Could you fix this for me?
Flags: needinfo?(tihuang) → needinfo?(honzab.moz)
Can you just land w/o it?  I don't have time to deal with it now, sorry.
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 96

a year ago
Created attachment 8833179 [details] [diff] [review]
Part 1: Add the nsILoadContextInfo as one additional argument of nsILoadContextInfo.onCacheEntryInfo() and CacheStorageService::EntryInfoCallback.OnEntryInfo().
Attachment #8833179 - Flags: review+
(Assignee)

Comment 97

a year ago
Created attachment 8833180 [details] [diff] [review]
part 2: Add a new interface nsICacheStorageService.asyncVisitAllStorages() to allow visiting every cache entry.
Attachment #8833180 - Flags: review+
(Assignee)

Comment 98

a year ago
Created attachment 8833181 [details] [diff] [review]
Part 3: Add a test for nsICacheStorageService.asyncVisitAllStorages()
Attachment #8833181 - Flags: review+
(Assignee)

Comment 99

a year ago
Created attachment 8833182 [details] [diff] [review]
Part 4: Add a new functoin in nsICacheEntry to allow its loadInfoContext can be fetched.
Attachment #8833182 - Flags: review+
(Assignee)

Comment 100

a year ago
Created attachment 8833183 [details] [diff] [review]
Part 5: Have the network predictor use OriginAttributes to properly partition connections it creates like non-predictive connections.
Attachment #8833183 - Flags: review+
(Assignee)

Comment 101

a year ago
Created attachment 8833184 [details] [diff] [review]
Part 6: Updating all callers of network predictor, and the docshell will update the first party domain if it is a typeContent mozbrowser.
Attachment #8833184 - Flags: review+
(Assignee)

Comment 102

a year ago
Created attachment 8833185 [details] [diff] [review]
Part 7: Updating the tests of network predictor.
Attachment #8833185 - Flags: review+
(Assignee)

Comment 103

a year ago
Created attachment 8833187 [details] [diff] [review]
Part 8: Updating speculativeConnect to speculativeConnect2 for about:newtab.
Attachment #8833187 - Flags: review+
(Assignee)

Comment 104

a year ago
Created attachment 8833188 [details] [diff] [review]
Part 9: Updating speculativeConnect to speculativeConnect2 for the search service.
Attachment #8833188 - Flags: review+
(Assignee)

Comment 105

a year ago
Created attachment 8833189 [details] [diff] [review]
Part 10: Add an assertion to make sure every code path of gecko uses speculativeConnect2 correctly.
Attachment #8833189 - Flags: review+
(Assignee)

Comment 106

a year ago
Created attachment 8833190 [details] [diff] [review]
Part 11: Updating speculativeConnect to speculativeConnect2 for Fennec.
Attachment #8833190 - Flags: review+
(Assignee)

Comment 107

a year ago
Created attachment 8833192 [details] [diff] [review]
Part 12: Updating speculativeConnect to speculativeConnect2 for test_speculative_connect.js.

Carrying r+ from Honza, smaug, Gijs, nwgh and sebastian.

Tomcat, could you land these patches and ignore patches from Mozreview?
Attachment #8833192 - Flags: review+

Comment 108

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9afc784f0bfb
Part 1: Add the nsILoadContextInfo as one additional argument of nsILoadContextInfo.onCacheEntryInfo() and CacheStorageService::EntryInfoCallback.OnEntryInfo(). r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f75a92ef485
part 2: Add a new interface nsICacheStorageService.asyncVisitAllStorages() to allow visiting every cache entry. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/e81dce16b4df
Part 3: Add a test for nsICacheStorageService.asyncVisitAllStorages(). r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa73e382ed7c
Part 4: Add a new function in nsICacheEntry to allow its loadInfoContext to be fetched. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4dd7cb34be
Part 5: Have the network predictor use OriginAttributes to properly partition connections it creates like non-predictive connections. r=nwgh
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f1437a1332d
Part 6: Update all callers of network predictor, and the docshell will update the first party domain if it is a typeContent mozbrowser. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f84930363b55
Part 7: Update the tests of network predictor. r=nwgh
https://hg.mozilla.org/integration/mozilla-inbound/rev/579c3a59a5c4
Part 8: Update speculativeConnect to speculativeConnect2 for about:newtab. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/24383ae625f3
Part 9: Update speculativeConnect to speculativeConnect2 for the search service. r=Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a2384c071d
Part 10: Add an assertion to make sure every code path of gecko uses speculativeConnect2 correctly. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d351c70d995
Part 11: Update speculativeConnect to speculativeConnect2 for Fennec. r=sebastian
https://hg.mozilla.org/integration/mozilla-inbound/rev/a381b34e6055
Part 12: Update speculativeConnect to speculativeConnect2 for test_speculative_connect.js. r=mayhemer
Keywords: checkin-needed
This is a great work.  Tim, thanks for finishing this up!
Also thanks for all the reviewers.

Updated

a year ago
Duplicate of this bug: 1309816
You need to log in before you can comment on or make changes to this bug.