1354349 - Add a channel.isDocument() method

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Jason Duell]

We are often asked how to determine if a channel is for a top-level document load, and our answer has always been "see if the LOAD_DOCUMENT_URI loadFlag is set".  It turns out that's not enough. See bug 1331680 comment 28 and patch 9 of bug 1337056. We should add a method (to either nsNetUtil.h or perhaps to nsIChannel?) that extracts the code in patch 9 in ContentParent::TransmitPermsFor() so we don't duplicate this code all over the tree.

Let's make sure to

1) replace the code in TransmitPermsFor() to use the new method

2) Change the docs in nsIChannel for LOAD_DOCUMENT_URI to note that while setting this flag is sufficient to mark a channel as a document load, /checking/ whether the channel is a document load requires the use of the new method.

3) a code audit of the tree for uses of LOAD_DOCUMENT_URI would be a good idea--there's a good chance that some of them should be using this new method instead.  

We're likely going to want this for bug 1331680, so we might as well do it now.

Comment 1

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=50b7e10e61420a7e0b29d544b8a3839461d5bc3e

Comment 2

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b8b42a3c3638baa23c6f248fa897a8b1504f77d

Comment 3

[Jason Duell]

When this is landed it's probably worth a short note on dev.platform to train people.  There are a lot of folks out there who are used to just using LOAD_DOCUMENT_URI.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1354349 - Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type

Review commit: https://reviewboard.mozilla.org/r/130040/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130040/

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1354349 - Use channel.isDocument in ContentParent::TransmitPermissionsFor

Review commit: https://reviewboard.mozilla.org/r/130042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130042/

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8857993 [details]
Bug 1354349 - Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type

https://reviewboard.mozilla.org/r/130040/#review132588

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8857993 [details]
Bug 1354349 - Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type

https://reviewboard.mozilla.org/r/130040/#review132590

::: dom/jsurl/nsJSProtocolHandler.cpp
(Diff revision 1)
>      NS_DECL_NSISCRIPTCHANNEL
>      NS_FORWARD_SAFE_NSIPROPERTYBAG(mPropertyBag)
>      NS_FORWARD_SAFE_NSIPROPERTYBAG2(mPropertyBag)
>  
>      nsresult Init(nsIURI *aURI, nsILoadInfo* aLoadInfo);
> -

nit: Please remove this unnecessary line deletion.

::: dom/jsurl/nsJSProtocolHandler.cpp:445
(Diff revision 1)
>  
>      return rv;
>  }
>  
> +
> +NS_IMETHODIMP

nit: Other methods only have one line of whitespace around them in this file.

::: netwerk/base/nsIChannel.idl:356
(Diff revision 1)
> +     * Returns true if the channel is used to create a document.
> +     * It returns true if the loadFlags have LOAD_DOCUMENT_URI set, or if
> +     * LOAD_HTML_OBJECT_DATA is set and the channel has the appropriate
> +     * MIME type.
> +     */
> +    readonly attribute bool isDocument;

We should probably note here that calling this function may return the wrong value before OnStartRequest (as we may not know the MIME type yet).

::: netwerk/base/nsIChannel.idl:359
(Diff revision 1)
> +     * MIME type.
> +     */
> +    readonly attribute bool isDocument;
> +
>  %{ C++
> +  inline bool IsDocument()

Why did you choose to write the wrapper this way instead of marking isDocument as [infallible] and handling errors by returning false in the implementation?

::: netwerk/base/nsNetUtil.cpp:372
(Diff revision 1)
> +  // document, depending on its MIME type.
> +
> +  if (!aChannel || !aIsDocument) {
> +      return NS_ERROR_NULL_POINTER;
> +  }
> +  nsLoadFlags loadFlags;

nit: I would prefer it if we set aIsDocument to false here, before we return any errors, just in case someone forgets to check the return value.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8857994 [details]
Bug 1354349 - Use channel.isDocument in ContentParent::TransmitPermissionsFor

https://reviewboard.mozilla.org/r/130042/#review132602

::: dom/ipc/ContentParent.cpp:5036
(Diff revision 1)
> -  nsLoadFlags loadFlags;
> -  nsresult rv = aChannel->GetLoadFlags(&loadFlags);
> -  NS_ENSURE_SUCCESS(rv, rv);
>  
> -  if (!(loadFlags & nsIChannel::LOAD_DOCUMENT_URI)) {
> -    if (loadFlags & nsIRequest::LOAD_HTML_OBJECT_DATA) {
> +  nsresult rv;
> +  if (!aChannel->IsDocument()) {

Thanks for making this much nicer :)

::: dom/jsurl/nsJSProtocolHandler.cpp:621
(Diff revision 1)
>          // LOAD_DOCUMENT_URI flag, so a docloader we're loading in as the
>          // document channel will claim to not be busy, and our parent's onload
>          // could fire too early.
>          nsLoadFlags loadFlags;
>          mStreamChannel->GetLoadFlags(&loadFlags);
> -        if (loadFlags & LOAD_DOCUMENT_URI) {
> +        if (mStreamChannel->IsDocument()) {

Do we know our MIME type to take advantage of IsDocument() yet at this point?

::: dom/jsurl/nsJSProtocolHandler.cpp:739
(Diff revision 1)
>      mStreamChannel->GetLoadFlags(&loadFlags);
>  
>      uint32_t disposition;
>      if (NS_FAILED(mStreamChannel->GetContentDisposition(&disposition)))
>          disposition = nsIChannel::DISPOSITION_INLINE;
> -    if (loadFlags & LOAD_DOCUMENT_URI && disposition != nsIChannel::DISPOSITION_ATTACHMENT) {
> +    if (mStreamChannel->IsDocument() && disposition != nsIChannel::DISPOSITION_ATTACHMENT) {

nit: flip the arguments to && here - IsDocument is more expensive than the disposition check so it seems like we should do it second.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

(In reply to Michael Layzell [:mystor] from comment #7)

> > +    readonly attribute bool isDocument;
> > +
> >  %{ C++
> > +  inline bool IsDocument()
> 
> Why did you choose to write the wrapper this way instead of marking
> isDocument as [infallible] and handling errors by returning false in the
> implementation?

[infallible] only works on builtinclass types.

(In reply to Michael Layzell [:mystor] from comment #8)
> > -        if (loadFlags & LOAD_DOCUMENT_URI) {
> > +        if (mStreamChannel->IsDocument()) {
> 
> Do we know our MIME type to take advantage of IsDocument() yet at this point?

We do, but I missed that mStreamChannel will always have text/html as a content type, so I don't really need to call isDocument here.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8857993 [details]
Bug 1354349 - Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130040/diff/1-2/

Comment 11

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8857994 [details]
Bug 1354349 - Use channel.isDocument in ContentParent::TransmitPermissionsFor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130042/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec9fb39f7133
Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type r=mcmanus,mystor
https://hg.mozilla.org/integration/autoland/rev/a28aa86d1ffe
Use channel.isDocument in ContentParent::TransmitPermissionsFor r=mystor

Comment 13

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/185945b7ea63
Backed out changeset a28aa86d1ffe 
https://hg.mozilla.org/integration/autoland/rev/9c7ae09cb1cb
Backed out changeset ec9fb39f7133 for bustage

Comment 14

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=92245557&repo=autoland

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8857993 [details]
Bug 1354349 - Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130040/diff/2-3/

Comment 16

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8857994 [details]
Bug 1354349 - Use channel.isDocument in ContentParent::TransmitPermissionsFor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130042/diff/2-3/

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8857993 [details]
Bug 1354349 - Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130040/diff/3-4/

Comment 18

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8857994 [details]
Bug 1354349 - Use channel.isDocument in ContentParent::TransmitPermissionsFor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130042/diff/3-4/

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8857993 [details]
Bug 1354349 - Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130040/diff/4-5/

Comment 20

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8857994 [details]
Bug 1354349 - Use channel.isDocument in ContentParent::TransmitPermissionsFor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/130042/diff/4-5/

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b01181410a8a
Add nsIChannel.isDocument that checks if LOAD_DOCUMENT_URI is set, or if LOAD_HTML_OBJECT_DATA and the channel has the appropriate MIME type r=mcmanus,mystor
https://hg.mozilla.org/integration/autoland/rev/8a42c7e1f799
Use channel.isDocument in ContentParent::TransmitPermissionsFor r=mystor

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b01181410a8a
https://hg.mozilla.org/mozilla-central/rev/8a42c7e1f799