Closed Bug 1312954 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: DOM: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: timhuang, Assigned: timhuang)

References

(Blocks 2 open bugs)

Details

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

Attachments

(24 files)

59 bytes, text/x-review-board-request
mayhemer
: review+
Details
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
59 bytes, text/x-review-board-request
u408661
: review+
Details
59 bytes, text/x-review-board-request
smaug
: review+
Details
59 bytes, text/x-review-board-request
u408661
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
Gijs
: review+
Details
59 bytes, text/x-review-board-request
ckerschb
: review+
Details
59 bytes, text/x-review-board-request
sebastian
: review+
Details
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
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
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)?
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.
Whiteboard: [tor]
Whiteboard: [tor] → [tor] [domsecurity-backlog1]
Blocks: 1309816
Assignee: nobody → tihuang
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]
Whiteboard: [tor] [domsecurity-active] → [tor] [domsecurity-active][OA]
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.
Summary: Making the network predictor obeys originAttriubtes and updating SpeculativeConnect() to SpeculativeConnect2(). → Making the network predictor obey originAttributes and updating SpeculativeConnect() to SpeculativeConnect2().
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 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+
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 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 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-
(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 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 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 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 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 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-
(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 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 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 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 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 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 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)
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 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 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 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 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+
> 
> 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 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+
Attachment #8828722 - Flags: review?(s.kaspari)
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 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+
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)
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)
Carrying r+ from Honza, smaug, Gijs, nwgh and sebastian.

Tomcat, could you land these patches and ignore patches from Mozreview?
Attachment #8833192 - Flags: review+
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: