1264137 - Set correct requestingContext passed to nsIContentPolicy in nsDocShell.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Tanvi Vyas[:tanvi]]

Attachment: docshell-contentpolicy-changes-04-12-16.patch

After discussion with Jonas and BillM, separating out the requestingContext changes from Bug 1105556.  Bill is worried that changing the behavior of requestingContext passed to nsIContentPolicy for TYPE_DOCUMENT loads may break addons.  Hence, we will basically leave it as is and just be more explicit in the code about what we are setting it to:

TYPE_DOCUEMNT:
if (content-process && e10s) { 
  use mscriptglobal
} else {
   use mscriptglobal->frame element internal
}

Attached is a WIP patch.  I'm having trouble checking for e10s mode though.  Is there a file I need to include in order to use mozilla::BrowserTabsRemoteAutostart()?

Comment 1

[Tanvi Vyas[:tanvi]]

Attachment: docshell-contentpolicy-changes-04-12-16B.patch

Jonas, is this what you had in mind?  If so, I'll also pass to BillM and Boris for review.  Thanks!

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8740706 [details] [diff] [review]
docshell-contentpolicy-changes-04-12-16B.patch

Review of attachment 8740706 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed. I don't think the |mozilla::BrowserTabsRemoteAutostart()| should be needed.

::: docshell/base/nsDocShell.cpp
@@ +9650,5 @@
>    // Only abort the load if a content policy listener explicitly vetos it!
> +
> +  nsISupports* requestingContext;
> +  nsCOMPtr<Element> requestingElement;
> +  if (XRE_IsContentProcess() && mozilla::BrowserTabsRemoteAutostart()) {

Why the |mozilla::BrowserTabsRemoteAutostart()| part?

@@ +9707,5 @@
>        nsIContentPolicy::TYPE_INTERNAL_IFRAME : nsIContentPolicy::TYPE_INTERNAL_FRAME;
> +
> +    // Get the docshell type for requestingElement
> +    nsCOMPtr<nsIDocument> requestingDoc = requestingElement->OwnerDoc();
> +    nsCOMPtr<nsIDocShell> elementDocShell = requestingDoc->GetDocShell();

These two should be #ifdef DEBUG along with the assertion below, since they only seem to exist to support the assertion.

Comment 3

[Tanvi Vyas[:tanvi]]

(In reply to Jonas Sicking (:sicking) from comment #2)
> Comment on attachment 8740706 [details] [diff] [review]
> docshell-contentpolicy-changes-04-12-16B.patch
> 
> Review of attachment 8740706 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with that fixed. I don't think the
> |mozilla::BrowserTabsRemoteAutostart()| should be needed.
> 
> ::: docshell/base/nsDocShell.cpp
> @@ +9650,5 @@
> >    // Only abort the load if a content policy listener explicitly vetos it!
> > +
> > +  nsISupports* requestingContext;
> > +  nsCOMPtr<Element> requestingElement;
> > +  if (XRE_IsContentProcess() && mozilla::BrowserTabsRemoteAutostart()) {
> 
> Why the |mozilla::BrowserTabsRemoteAutostart()| part?
>

We said if we are in the child process in e10s mode, then use mScriptGlobal.  The BrowserTabsRemoteAutostart is to check if we are in e10s mode.  There are circumstances where XRE_IsContentProcess() will return true in non-e10s mode (i.e. to process thumbnails).  Since most likely those won't matter for docshell loads, I could take that part out if you'd like.  Let me know.  In the meantime, I'll update the patch and r? to billm and bz.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, I'd say take that out. Most likely we should use mScriptGlobal any time we're in a child process, not just when Firefox has flipped the e10s pref.

Comment 5

[Tanvi Vyas[:tanvi]]

Attachment: docshell-contentpolicy-changes-04-14-16.patch

Updated the patches per Jonas' comments, but the assertion for requestingContext fails on browser startup:

https://pastebin.mozilla.org/8867779

It fails in the else case where we do for !XRE_IsContentProcess():
    requestingElement = mScriptGlobal->AsOuter()->GetFrameElementInternal();
    requestingContext = requestingElement;

mScriptGlobal still exists.  Are there cases where we don't have the frame element in the parent process?  Should we be passing mScriptGlobal in those cases?  needinfo'ing Bill and Jonas to see if one of them can advise.  Thanks!

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Doh! Of course it fires. It fires when we're creating the absolute top-level docshell and load browser.xul into that. That top-level docshell will of course not have any form of parents.

I'm happy to just remove the assertion. Or we could try

MOZ_ASSERT(requestingContext || nsContentUtils::IsSystemPrincipal(requestingPrincipal))

When the requestingPrincipal is systemPrincipal we won't actually call nsIContentPolicies anyway.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hrm.. that might need to be

MOZ_ASSERT(requestingContext || !requestingPrincipal ||
           nsContentUtils::IsSystemPrincipal(requestingPrincipal));

Comment 8

[Tanvi Vyas[:tanvi]]

(In reply to Jonas Sicking (:sicking) from comment #7)
> Hrm.. that might need to be
> 
> MOZ_ASSERT(requestingContext || !requestingPrincipal ||
>            nsContentUtils::IsSystemPrincipal(requestingPrincipal));

I assume you mean the requestingPrincipal, as created in line 9724 here: https://bugzilla.mozilla.org/attachment.cgi?id=8740706&action=diff#a/docshell/base/nsDocShell.cpp_sec4

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Won't this code do the wrong thing if we have an iframe in the child process? In that case we do want to use the window's owner element, don't we?

For Jonas's assertion, could we instead check that the docshell type is chrome and it has no parent? That seems a little stricter to me.

Comment 10

[Tanvi Vyas[:tanvi]]

Attachment: docshell-contentpolicy-changes-04-14-16B.patch

Comment 11

[Tanvi Vyas[:tanvi]]

Attachment: docshell-contentpolicy-changes-04-19-16.patch

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8743129 [details] [diff] [review]
docshell-contentpolicy-changes-04-19-16.patch

Review of attachment 8743129 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +9687,5 @@
>        }
>      }
>    }
>    if (IsFrame() && !isNewDocShell && !isTargetTopLevelDocShell) {
> +    // Use nsPIDOMWindow since we _want_ to cross the chrome boundary if needed

It doesn't seem like this comment is correct. We never cross the chrome boundary here. You're in fact asserting that we don't below.

I think you can just remove this comment.

@@ +9699,5 @@
> +#ifdef DEBUG
> +    // Get the docshell type for requestingElement
> +    nsCOMPtr<nsIDocument> requestingDoc = requestingElement->OwnerDoc();
> +    nsCOMPtr<nsIDocShell> elementDocShell = requestingDoc->GetDocShell();
> +    

Remove whitespace

Comment 13

[Tanvi Vyas[:tanvi]]

Try shows 3 crashes with the requestingContext assert in the patch that I need to take a look at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=15d3873813db&selectedJob=19707587

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Might be worth looking into why the assert is failing. But ultimately i'd be totally happy to drop the assertion. Things are no worse than before this patch, and I mainly wanted the assertion to be there as documentation.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8743129 [details] [diff] [review]
docshell-contentpolicy-changes-04-19-16.patch

Review of attachment 8743129 [details] [diff] [review]:
-----------------------------------------------------------------

I apologize for how long this took.

::: docshell/base/nsDocShell.cpp
@@ +9696,5 @@
>      contentType = requestingElement->IsHTMLElement(nsGkAtoms::iframe) ?
>        nsIContentPolicy::TYPE_INTERNAL_IFRAME : nsIContentPolicy::TYPE_INTERNAL_FRAME;
> +
> +#ifdef DEBUG
> +    // Get the docshell type for requestingElement

Please end with a period.

@@ +9717,5 @@
>    }
>  
>    // XXXbz would be nice to know the loading principal here... but we don't
> +  // This behavior should be changed in bug 1181370. requestingPrincipal passed
> +  // to nsIContentPolicy shoudl equal triggeringPrincipal passed to loadInfo.

should

@@ +9727,5 @@
>    }
>  
> +  nsCOMPtr<nsIDocShellTreeItem> parent = GetParentDocshell();
> +  MOZ_ASSERT(requestingContext ||
> +             (mItemType == typeChrome && !parent),

The test failure is caused by the windowless browser [1] in browser_windowless_troubleshoot_crash.js. We could weaken the assertion to instead say (requestingContext || !parent), but that's not all that meaningful. Or we could try to find some way of singling out these windowless docshells. Maybe check the IsInvisible property.

I agree with Jonas that the assertion doesn't really matter, so please do whatever makes sense to you.

[1] http://searchfox.org/mozilla-central/source/xpfe/appshell/nsAppShellService.cpp#498

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, I'd say just remove the assertion and check this in.

Comment 17

[Tanvi Vyas[:tanvi]]

Attachment: docshell-contentpolicy-changes-04-26-16.patch

Thanks Bill and Jonas!

Removed requestingContext assert and fixed review comments.  Carrying over r+ from Bill and Jonas, but I'm also going to r? to Boris.

Also pushed to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a7a94006e64

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8745682 [details] [diff] [review]
docshell-contentpolicy-changes-04-26-16.patch

I don't understand what this code is doing either before or after this patch.

In the case when targetDocShell is not null, why do we care what IsFrame() says, and in what way is the requestingElement mScriptGlobal->AsOuter()->GetFrameElementInternal()?  This doesn't really make sense.

Of course, in the case when targetDocShell is not null, I don't see why we even do a content policy check in _this_ call at all.  :(  It's pretty nonsensical.  If targetDocShell is null and isNewDocShell, I can almost kinda see doing the check before we start opening windows, I guess.  But in the other case, it's just pointless.

Anyway, r- because if nothing else this needs a commit message that actually explains what the intended behavior is in the various cases here, including the ones where the load will not actually be happening in "this".  Given that I would be able to at least evaluate the rest of the patch....

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: rename to requestingPrincipal and requestingContext

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: Refactor existing code.

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: Set corret requestingContext and pass it to nsIContentPolicy

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: Set corret requestingContext and pass it to nsIContentPolicy

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

So I have breaked Tanvi's patch into 3 parts accordingly, the first 2 parts are just renaming/refactoring, the last part is what Tanvi tries to fix in the beginning.

Since bz needs some explaination, so I've added some notes in the commit message.

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Bz
You reviewed the original patch in Comment 18, and gave a r-.
Now I took this bug over, and have breaked the original patches into 3 parts to make it easier to review.

I'd like to know if you still want to review this? 
Or it's fine I find another DOM peer to review this.

Thanks

Comment 25

[Boris Zbarsky [:bzbarsky]]

I can probably review this, but the patches don't seem to address my concerns from comment 18...

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: perform ContentPolicy check if the load is happening on this docshell.

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4. pass requestingContext to nsIContentPolicy

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi bz
(In reply to Boris Zbarsky [:bz] (busy, pun intended) from comment #18)
> In the case when targetDocShell is not null, why do we care what IsFrame()
> says, 
Thanks for your suggestion, so in Part 3 I only call nsIContentPolicy if it happens on 'this' docshell.

> and in what way is the requestingElement
> mScriptGlobal->AsOuter()->GetFrameElementInternal()?  
> 
In part 4 there are some notes explaining that.

Would like to hear you comments on this.

Thanks

Comment 29

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here.  I'm aiming to get to this tomorrow morning....

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8790219 [details] [diff] [review]
Part 1: rename to requestingPrincipal and requestingContext

The commit message should probably say _what_ is being renamed and where; it's not a global rename or anything...

The code parts of this are fine.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8790220 [details] [diff] [review]
Part 2: Refactor existing code.

The first line of the commit message really needs to actually say something useful.  Maybe this:

  Bug 1264137 - Part 2: get rid of the useless isNewDocShell variable and just keep track of isTargetTopLevelDocShell, since that's all we care about.

or something like that.

r=me with that.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8790716 [details] [diff] [review]
Part 3: perform ContentPolicy check if the load is happening on this docshell.

OK.  This is much more plausible, as long as it does not break addons.  You'll have to update the test more carefully (e.g. fix the comments in it).

You shouldn't need to determine the requesting element and content type if !targetDocShell, but I guess it's cheap enough and not worth the "maybe uninitialized" compiler warnings if you did it that way.  Please add a comment explaining why the computation is done unconditionally, though?

Also document that you have to do that before you possibly-delegate to a different target docshell, because you want to do this before opening new windows if new windows are required.

>+  // First, notify any nsIContentPolicy listeners about the document load.

Except this is happening conditionally now, right?  Seems like the comments should reflect that.

r=me with the test fixed more thoroughly, the comments here improved, and an addon-compat annotation plus maybe pinging :jorge.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8790718 [details] [diff] [review]
Part 4. pass requestingContext to nsIContentPolicy

Again, this needs a much better commit message.

>+    nsCOMPtr<Element> requestingElement;
>     nsISupports* requestingContext = requestingElement;

That second line is a really complicated way of writing:

  nsISupports* requestingContext = nullptr;

and I would prefer the simpler version.  ;)

>+       // This happens when loading browser.xul.

Or more precisely, when loading browser.xul we will in fact have contentType == TYPE_DOCUMENT and will have !XRE_IsContentProcess(), but we will also have a null mScriptGlobal->AsOuter()->GetFrameElementInternal().  So claiming that this case is just about browser.xul is pretty misleading: if it were we'd just set requestingElement = requestingContext = nullptr and then move on.

This case is really about non-e10s loads in tabs.  Those might be actual non-e10s sessions or any of the various Firefox UI bits that we force into non-e10s mode when loaded (about:addons or whatnot).  Please adjust the comments accordingly.

Also, is it intentional that you're changing the browser.xul case to have a null requestingContext?  Maybe that's what the comment is trying to say?  If so, it should be a lot clearer about it...

I would like to see and updated patch with better commit message and comments here before marking r+.

Comment 34

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Boris Zbarsky [:bz] (TPAC) from comment #32)
> You shouldn't need to determine the requesting element and content type if
> !targetDocShell,
Do you mean
"You shouldn't need to determine the requesting element and content type if *targetDocShell*"?
Like I should only compute requestingElement and contentType inside if (!targetDocShell)?
if so, contentType variable is still being used later when we call DoURILoad
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10578

and contentType needs to know if it's iframe base on requestingElement.

Comment 35

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Huang[:allstars.chh] from comment #34)
> (In reply to Boris Zbarsky [:bz] (TPAC) from comment #32)
> and contentType needs to know if it's iframe base on requestingElement.
Sorry, I got your point, will add some notes for requestingElement.

Comment 36

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: rename from loadingPrincipal to requestingPrincipal in nsDocShell::InternalLoad. v2

Comment 37

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: use isTargetTopLevelDocShell instead. v2

Comment 38

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: perform ContentPolicy check if the load is happening on this docshell. v2

Comment 39

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: pass requestingContext to nsIContentPolicy v2

removed the bogus comment "loading browser.xul"

Comment 40

[Boris Zbarsky [:bzbarsky]]

> "You shouldn't need to determine the requesting element and content type if *targetDocShell*"?

Yes, sorry for getting the boolean the wrong way around.

> Like I should only compute requestingElement and contentType inside if (!targetDocShell)?

That would be possible, yes.  It may not be desirable, but I'm just asking for it to be documented why it's not desirable.

> contentType variable is still being used later when we call DoURILoad

Yes, but that only happens if !targetDocShell.

So you _could_ structure the code like this:

  uint32_t contentType;
  if (!targetDocshell) {
    // compute contentType.
    // Do content policy check.
  }
  // Kick load over to another docshell as needed.
  DoURILoad();

The problem, as I said, is that I will bet compilers will warn that contentType may not be initialized when used in DoURILoad, though in practice it always is in this case.  So we should move the computation out of the "if" but document why we're doing it even though it's not needed in some cases.

I just realized that there is one weird result of this patch: in the target=foo case we will still do two content policy checks: once in the original docshell and once in the new one when initially opening the window, whereas targeted loads that find an existing docshell will only do the one check in the target.  I don't see a good way around that, so I guess that's ok.

Comment 41

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: rename from loadingPrincipal to requestingPrincipal in nsDocShell::InternalLoad. v3

rebase

Comment 42

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: perform ContentPolicy check if the load is happening on this docshell. v3

rebase base on bug 1246540.
Since in this patch we only perform CSP check when the load happens in 'this' docshell, I also modified the call of checking HSTS priming when the load is happening in 'this' docshell, r? for smaug as he reviewed bug1246540.

Comment 43

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8797566 [details] [diff] [review]
Part 3: perform ContentPolicy check if the load is happening on this docshell. v3

I think I will split this patch as bz already reviewed part of the patch.

Comment 44

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: perform ContentPolicy check if the load is happening on this docshell. v2

Comment 45

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3.1: check for HSTS priming if it happens on this docshell.

rebase on bug 1246540, r? smaug as he reviewed bug 1246540.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8797571 [details] [diff] [review]
Part 3.1: check for HSTS priming if it happens on this docshell.

I'm having trouble to review this since it doesn't apply to the current m-c, so I don't quite know the context here.
You're moving code to be inside some other 'if', but which one, and why? Could you explain and ask review again.

Comment 47

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #46)
> I'm having trouble to review this since it doesn't apply to the current m-c,
> so I don't quite know the context here.
> You're moving code to be inside some other 'if', but which one, and why?
> Could you explain and ask review again.
This patch needs:
Part 1 to rename context to requestingContext
Part 3 call nsIContentPolicy if the load happens on *this* docshell, as bz commented in Comment 18, if there's a targetDocShell then the check should be done in that targetDocShell.

I assume that HSTS priming should be done in that same way?
or it should be done whether targetDocShell is null or not?

Or if you think I should let bz review this first I'll redirect r? to him.

Comment 48

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: pass requestingContext to nsIContentPolicy v3

update comments and commit message.

Comment 49

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8797980 [details] [diff] [review]
Part 4: pass requestingContext to nsIContentPolicy v3

I found bz doesn't accept a r? now, ni? him when he has time on this.
Thanks

Comment 50

[Boris Zbarsky [:bzbarsky]]

How about this for the commit message:

   Bug 1264137 - Part 4: Improve the usefulness of what we pass as requestingContext to nsIContentPolicy for navigations (loads in a docshell).

  For toplevel document loads (TYPE_DOCUMENT) in the content process
  we pass the currently-loaded window, if any.

  For toplevel document loads in the chrome process (e.g. tabs in
  non-e10s mode), we pass the node which created our docshell, if any.

  For all subframe loads, we pass the node that created the docshell, which is the frameElement of the window in the docshell.

Comment 51

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8797980 [details] [diff] [review]
Part 4: pass requestingContext to nsIContentPolicy v3

>+        // In e10s the child process doesn't have access to the window,

You mean it doesn't have access to the element that contains the browsing context (because that element is in the chrome process).  Please fix the commment.

>+        // This is for loading browser.xul and non-e10s tab.

Or any other of a bajillion toplevel windows (view-source, page info, etc, etc).  How about saying:

  // This is for loading non-e10s tabs and toplevel windows of various sorts.
  // For the toplevel window cases, requestingElement will be null.

>+    // When loading browser.xul, requestingContext will be null.

And in all sorts of other cases too.  Please adjust the comment accordingly.  Perhaps something like:

  // When loading toplevel windows, requestingContext can be null.  We don't
  // really care about HSTS in that situation, though; loads in toplevel
  // windows should all be browser UI.

r=me with those fixes.

Comment 52

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: Improve the usefulness of what we pass as requestingContext to nsIContentPolicy for navigations (loads in a docshell).

Comment 53

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8797571 [details] [diff] [review]
Part 3.1: check for HSTS priming if it happens on this docshell.

Review of attachment 8797571 [details] [diff] [review]:
-----------------------------------------------------------------

Hi smaug, please see comment 47 for explanation.

Comment 54

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: Improve the usefulness of what we pass as requestingContext to nsIContentPolicy for navigations (loads in a docshell). v4

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8797571 [details] [diff] [review]
Part 3.1: check for HSTS priming if it happens on this docshell.

ok, makes sense since if there is targetDocShell, we'll call InternalLoad again.
(someone could re-architect this code a bit to make it more readable)

Comment 56

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: perform ContentPolicy check if the load is happening on this docshell. v3

merged into one patch.

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c579502f3ef6
Part 1: rename from loadingPrincipal to requestingPrincipal in nsDocShell::InternalLoad. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/a665a8f5a2a7
Part 2: use isTargetTopLevelDocShell instead. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d13ef7f6be4
Part 3: perform ContentPolicy check if the load is happening on this docshell. r=bz, smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3ff2ee8dc2
Part 4: Improve the usefulness of what we pass as requestingContext to nsIContentPolicy for navigations (loads in a docshell). r=bz

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c579502f3ef6
https://hg.mozilla.org/mozilla-central/rev/a665a8f5a2a7
https://hg.mozilla.org/mozilla-central/rev/5d13ef7f6be4
https://hg.mozilla.org/mozilla-central/rev/7c3ff2ee8dc2