909218 - Allow a docShell to have default load flags that will be applied to every request made from it.

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

There are some use-cases for being able to specify that a docShell is "anonymous", which translates to "use the LOAD_ANONYMOUS flag for all requests made by this docShell"

A little more context at https://groups.google.com/forum/#!topic/mozilla.dev.tech.network/X8t4Sob3l9U

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-909218-Add-isAnonymous-flag-to-nsIDocShell-and-p.patch

This patch adds an isAnonymous attribute to nsIDocShell and tries to use it correctly - hopefully somewhat as described in the dev.tech.network thread.

FTR, it seems likely that the same basic use-case we have might want an extra flag or 2 to be treated in the same way (eg, LOAD_BYPASS_CACHE).  I only mention this incase it changes what approach we should use (eg, maybe we should call the flag something like |isIsolated| (isPrivate would be confusing WRT private-browsing mode) so additional LoadFlags could be added later behind the same single flag)

Anyway - this patch is mainly to get the ball rolling...

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 795294 [details] [diff] [review]
0001-Bug-909218-Add-isAnonymous-flag-to-nsIDocShell-and-p.patch

>+    // XXX - should we enumerate existing children like IsActive does above?

My gut feeling is yes.

>+        // but maybe the parent is

We wouldn't need this if we propagated the set to kids, right?

>+   * before the docShell loads any content. The default is that docShell's are

s/docShell's/docShells/

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Attachment: Add 'loadFlags' attribute to nsIDocShell

This is an alternative, more general patch that adds 'loadFlags' to nsIDocShell, but is otherwise similar to the previous patch (but with feedback comments addressed).  This patch would allow us to add flags other than LOAD_ANONYMOUS.

An obvious issue with this is that the patch to nsLoadGroup still only passes LOAD_ANONYMOUS, whereas we are likely to want a few others too - eg, possibly LOAD_BYPASS_CACHE and INHIBIT_PERSISTENT_CACHING, which we could discuss in a followup.

Note I haven't actually tested this yet - I just thought I'd run it up the flagpole and see if bz salutes :)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 796488 [details] [diff] [review]
Add 'loadFlags' attribute to nsIDocShell

This seems ok, but maybe we should call these defaultLoadFlags or something?

Comment 5

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-first-cut-at-adding-nsIDocShell.loadFlags.patch

Updated to use nsIDocShell.defaultLoadFlags

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-add-defaultLoadFlags-to-nsILoadGroup.patch

This patch adds nsILoadGroup::defaultLoadFlags, and these flags are always "merged" into new requests for this docShell.  Setting nsIDocShell::defaultLoadFlags passes the flags onto the docShell's mLoadGroup.

Note that LOAD_ANONYMOUS is no longer included in the "default" merged flags - that could be a followup if desired, but with this patch you get the same effect by adding LOAD_ANONYMOUS to the docShell's defaultLoadFlags.

First cut at tests following...

Comment 7

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-909218-tests.patch

Tests - we load an iframe containing a HTML file that references .css, .js and .png resources.  Before loading this, we add a progress listener and check the request flags on each of the requests have been propagated correctly from the docShell.

I'll probably expand the test to include a XHR request and a sub-iframe to ensure the flags also end up correctly on them (but I've no reason to believe they will not get them too)

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-add-defaultLoadFlags-to-nsILoadGroup.patch

Minor fix - unsaved editor buffer!

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 797679 [details] [diff] [review]
0003-Bug-909218-tests.patch

Seems fine.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 797847 [details] [diff] [review]
0002-add-defaultLoadFlags-to-nsILoadGroup.patch

Seems ok, but I'd like a necko peer to review.

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 797847 [details] [diff] [review]
0002-add-defaultLoadFlags-to-nsILoadGroup.patch

Honza,
  I picked you (almost) at random as a Necko peer.  Can you give high-level feedback on this patch so I can be confident it's worth spending more time fleshing out the tests etc?  Obviously, feel free to pass this on to another Necko peer if desired.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 797847 [details] [diff] [review]
0002-add-defaultLoadFlags-to-nsILoadGroup.patch

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

::: docshell/base/nsDocShell.cpp
@@ +5404,5 @@
> +    // and if we have a load group, we add these bits to the flags that should
> +    // be set on all requests in this group.
> +    if (mLoadGroup) {
> +        mLoadGroup->SetDefaultLoadFlags(aDefaultLoadFlags);
> +    }

We may want an NS_WARNING() when the load group is not here (however, it shouldn't happen according that it is created in docshell's xpcom constructor - NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsDocShell, Init)).

::: netwerk/base/public/nsILoadGroup.idl
@@ +82,5 @@
>      readonly attribute nsILoadGroupConnectionInfo connectionInfo;
> +
> +    /**
> +     * The set of load flags that will be added to all requests in this group.
> +     */

I lack a bit more documentation here (or it just needs to be made more clear):

- are the flags added to all request currently in the group?
- or are the flags added only to new requests only?
- when I set a flag with this API and then remove it later again with this API, what happens?

This is all clear when looking into the code, but not when looking only at this comment.  Few words about the flags being inherited would be good too..

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 797675 [details] [diff] [review]
0001-first-cut-at-adding-nsIDocShell.loadFlags.patch

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

::: docshell/base/nsDocShell.cpp
@@ +5391,5 @@
> +{
> +    mDefaultLoadFlags = aDefaultLoadFlags;
> +    // Recursively tell all of our children.  We *do not* skip
> +    // <iframe mozbrowser> children - if someone sticks custom flags in this
> +    // docShell then they too get treated as anonymous.

if someone sticks custom flags in this
		// docShell then they too get treated as anonymous

Hmm... this doesn't sound right :)

and also opens a question whether we should somehow filter the load flags, I personally don't have an opinion tho...

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 797679 [details] [diff] [review]
0003-Bug-909218-tests.patch

Thanks!  Any thoughts on the tests - eg, what else should be covered or a better approach?

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 797679 [details] [diff] [review]
0003-Bug-909218-tests.patch

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

Yeah:
- write reasonable detailed description (or list of steps) what the test is exactly doing, so understanding it is simpler
- mark where the tests start (entry point)

::: docshell/test/chrome/test_bug909218.html
@@ +43,5 @@
> +// an nsIWebProgressListener that checks all requests made by the docShell
> +// have the flags we expect.
> +RequestWatcher = {
> +  init: function(docShell, url, expectedRequests, callback) {
> +    this.url = url;

maybe call 'this.url' like 'this.documentURL' or 'rootURL' or so...

@@ +52,5 @@
> +          getInterface(Ci.nsIWebProgress).
> +          addProgressListener(this, Ci.nsIWebProgress.NOTIFY_STATE_REQUEST |
> +                                    Ci.nsIWebProgress.NOTIFY_STATE_DOCUMENT);
> +    this.requestCounts = {};
> +    for (var url of EXPECTED_REQUESTS) {

so, EXPECTED_REQUESTS or expectedRequests?

@@ +61,5 @@
> +  finalize: function() {
> +    ok(Object.keys(this.requestCounts).length, "we expected some requests");
> +    for (var url in this.requestCounts) {
> +      var count = this.requestCounts[url];
> +      ok(count >= 1, url + " saw " + count + " requests");

while exactly not == 1 ?

@@ +99,5 @@
> +  // downloadable stops loading.
> +  function basic() {
> +    var iframe = insertIframe();
> +    var docShell = docshellForWindow(iframe.contentWindow);
> +    RequestWatcher.init(docShell, TEST_URL, EXPECTED_REQUESTS, function() {

Instead EXPECTED_REQUESTS just inline the urls?

Comment 16

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-909218-add-defaultLoadFlags-to-nsIDocShell.-r-bz.patch

Very similar to previous patch with feedback addressed.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0002-Bug-909218-add-defaultLoadFlags-to-nsILoadGroup-and-.patch

This too is very similar to the previous one, with feedback addressed.

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0003-Bug-909218-test-defaultLoadFlags-make-it-to-all-requ.patch

This test is cleaned up significantly from the previous version.  Unfortunately I couldn't work out a reasonable way to ensure XHR requests made by the page also have the correct flags as the docShell's progress listener doesn't report them - but I did verify in the debugger that they do.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 799261 [details] [diff] [review]
0001-Bug-909218-add-defaultLoadFlags-to-nsIDocShell.-r-bz.patch

r=me

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 799262 [details] [diff] [review]
0002-Bug-909218-add-defaultLoadFlags-to-nsILoadGroup-and-.patch

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

r=honzab

thanks.

::: docshell/base/nsDocShell.cpp
@@ +5402,5 @@
>          docshell->SetDefaultLoadFlags(aDefaultLoadFlags);
>      }
> +    NS_ASSERTION(mLoadGroup, "We must have a load group");
> +    // Tell the load group to set these flags all requests in the group
> +    mLoadGroup->SetDefaultLoadFlags(aDefaultLoadFlags);

I liked the non-null check.

Nit: maybe for consistency set the load group flags before you iterate the children.  You first set default load flags on the docshell, then on children, they set in on their load groups, and after that you set it on the root load group.  But this is just a detail and if you have a reason to keep it like this, then do so.

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 799263 [details] [diff] [review]
0003-Bug-909218-test-defaultLoadFlags-make-it-to-all-requ.patch

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

::: docshell/test/chrome/test_bug909218.html
@@ +124,5 @@
> +
> +</script>
> +</head>
> +<body>
> +<p id="display">

What's this for?

Comment 22

[Mark Hammond [:markh] [:mhammond]]

Thanks!

Try at https://tbpl.mozilla.org/?tree=Try&rev=2c01a45f1fd4

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cd87c75e840
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e289a695226
https://hg.mozilla.org/integration/mozilla-inbound/rev/59926e3b88a0

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1cd87c75e840
https://hg.mozilla.org/mozilla-central/rev/9e289a695226
https://hg.mozilla.org/mozilla-central/rev/59926e3b88a0

Comment 24

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Mark, please make sure to actually note *all* the reviewers in the commit message.

Comment 25

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Ms2ger (away 11-21 September) from comment #24)
> Mark, please make sure to actually note *all* the reviewers in the commit
> message.

I believe I did.  Each patch had only one formal reviewer.  bz gave f+ to a number of patches, but only r+ to one of them and I mentioned that in that commit.  Honza gave r+ in 2 and I correctly attributed him in them (and in the non-test patch reviewed by Honza, bz explicitly asked for a Necko peer to review)