Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add a channel.isDocument() method

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking
RESOLVED FIXED
4 months ago
3 months ago

People

(Reporter: jduell, Assigned: valentin)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active])

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

4 months ago
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.
(Assignee)

Updated

4 months ago
Assignee: nobody → valentin.gosu
Whiteboard: [necko-next] → [necko-active]
(Assignee)

Comment 1

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=50b7e10e61420a7e0b29d544b8a3839461d5bc3e
(Assignee)

Comment 2

3 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b8b42a3c3638baa23c6f248fa897a8b1504f77d
(Reporter)

Comment 3

3 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

3 months ago
mozreview-review
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
Attachment #8857993 - Flags: review?(mcmanus) → review+

Comment 7

3 months ago
mozreview-review
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.
Attachment #8857993 - Flags: review?(michael) → review+

Comment 8

3 months ago
mozreview-review
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.
Attachment #8857994 - Flags: review?(michael) → review+
(Assignee)

Comment 9

3 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

3 months ago
Pushed by valentin.gosu@gmail.com:
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

3 months ago
Backout by cbook@mozilla.com:
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
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=92245557&repo=autoland
Flags: needinfo?(valentin.gosu)
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 21

3 months ago
Pushed by valentin.gosu@gmail.com:
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
(Assignee)

Updated

3 months ago
Flags: needinfo?(valentin.gosu)

Comment 22

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b01181410a8a
https://hg.mozilla.org/mozilla-central/rev/8a42c7e1f799
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.