1149853 - Track the RequestContext corresponding to each fetch in LoadInfo

Status: RESOLVED INCOMPLETE

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

I'm working on a huge patch set which stores a RequestContext member on mozilla::LoadInfo and requires us to set it everywhere that we create a channel.  This is intended to avoid changing nsContentPolicyTypes as discussed in bug 1148935.

Comment 1

[(no longer active)]

Attachment: Part 1: Store a RequestContext member in LoadInfo, and switch the C++ call sites where we create channels to pass a RequestContext

Note that with this patch, we may send the wrong RequestContext in some
places.  This will subsequently be fixed.

Also note that the JS XPIDL APIs changed here all allow the caller to
pass a RequestContext WebIDL enum value in order to make things easier.
We also pass a string value for the RequestContext from externally
linked code because RequestBinding.h cannot be #included in such code.

Comment 2

[(no longer active)]

Attachment: Part 2: Fix the JS callers of imgILoader.loadImageXPCOM

Comment 3

[(no longer active)]

Attachment: Part 3: Fix the JS callers of nsIIOService.newChannelFromURI2

Comment 4

[(no longer active)]

Attachment: Part 4: Fix the JS callers of nsIIOService.newChannel2

Comment 5

[(no longer active)]

Attachment: Part 5: Fix the JS callers of nsIWebSocketChannel.initLoadInfo

Comment 6

[(no longer active)]

Things are green on try so far: <https://treeherder.mozilla.org/#/jobs?repo=try&revision=f71c0e3a7783>

I will need to fix the bustage from new code racing with me landing this as it happens, and I expect most of the fixes will be very straightforward.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So the good news is that newChannel2/newChannelFromURI2 are not used by extensions yet.  Hopefully that will continue until we ship this. Unfortunately, we shipped those APIs already, in 36...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8587681 [details] [diff] [review]
Part 1: Store a RequestContext member in LoadInfo, and switch the C++ call sites where we create channels to pass a RequestContext

bkelly suggested that we should assert somewhere (e.g. in the newchannel code) that the content policy type and the request context match up properly.  Good idea for a followup.

sicking suggested that instead we should just change all these APIs to only pass in the RequestContext type and then derive the nsIContentPolicy type from that inside the loadinfo, say.

I kinda like sicking's idea.  Do you mind doing that?  That _does_ mean we have to actually make our RequestContext types correct, of course (some of the ones below are not; see review comments).

The other suggestion sicking had was to just add new types to nsIContentPolicy that consumers would pass in, but in the conpol service collapse those to the exising types before invoking content policy consumers.  This would also let us do the patches incrementally, I guess...

Also, apparently Christoph has outstanding patches that touch all this stuff too; please coordinate landing with him?

Code comments below, since I wrote most of them before the above discussion happened...

>+++ b/docshell/base/nsDocShell.cpp
>@@ -9628,16 +9631,17 @@ nsDocShell::InternalLoad(nsIURI* aURI,
>   if (IsFrame() && !isNewDocShell && !isTargetTopLevelDocShell) {
>     NS_ASSERTION(requestingElement, "A frame but no DOM element!?");
>     contentType = nsIContentPolicy::TYPE_SUBDOCUMENT;
>   } else {
>     contentType = nsIContentPolicy::TYPE_DOCUMENT;
>   }
>+  requestContext = RequestContext::Internal;

Why not set it based at least on the frame vs not distinction?  At the very
least, please file a followup bug on getting this right, and reference it here.

>@@ -10448,17 +10453,18 @@ nsDocShell::DoURILoad(nsIURI* aURI,
>+                      RequestContext aContext)

aRequestContext, please.  aContext is so overloaded in this code.  :(

>+++ b/docshell/base/nsDocShell.h
>+enum class RequestContext : uint32_t;

I wonder whether it's worth having a "declare an IDL enum" macro or something....  Followup, if so.

>+++ b/dom/base/EventSource.cpp
>+                       RequestContext::Xmlhttprequest,

RequestContext::Eventsource, no?

>+                       RequestContext::Xmlhttprequest,

And here.

>+++ b/dom/base/WebSocket.cpp
>+                          mozilla::dom::RequestContext::Internal);

Don't need the "mozilla::dom::" prefix here.  Also, it's pretty odd that there is no "websocket" request context... why not?  Followup bug, please.

>+++ b/dom/base/nsDocument.cpp
>@@ -1343,16 +1344,17 @@ nsExternalResourceMap::PendingLoad::StartLoad(nsIURI* aURI,
>+                     mozilla::dom::RequestContext::Internal,

No need for "mozilla::dom::". Also, this should not be "internal", since this is a totally per-spec load.  If there is no context type for this in the spec ("subresource"?  This load is for an SVG resource document.), please file a spec bug, a corresponding bug on our end, and reference it here.

>+++ b/dom/base/nsImageLoadingContent.cpp
>+nsImageLoadingContent::ContextForLoad(ImageLoadType aImageLoadType)

RequestContextForLoad, please.

>+++ b/dom/base/nsObjectLoadingContent.cpp
>+                     RequestContext::Object,

This needs to be Embed sometimes.  You should probably base that on thisContent->NodeInfo()-Equals(someAtom) or thisContent->IsHTML(someAtom).

The applet situation here is https://www.w3.org/Bugs/Public/show_bug.cgi?id=28360 I assume?  Please reference our bug tracking that here?

>+++ b/dom/base/nsScriptLoader.cpp
>+                     mozilla::dom::RequestContext::Script,

No need for "mozilla::dom::".

>+++ b/dom/html/HTMLMediaElement.cpp
>+                     mozilla::dom::RequestContext::Audio,

No need for "mozilla::dom::".  Also, sometimes Video.  Use IsVideo() to tell which one to use.

>+++ b/dom/html/nsHTMLDocument.cpp
>+                     mozilla::dom::RequestContext::Internal);

No need for "mozilla::dom::".

>+++ b/dom/jsurl/nsJSProtocolHandler.cpp
>+++ b/dom/media/MediaResource.cpp
>+                              mozilla::dom::RequestContext::Audio,

This is almost certainly wrong.  Please make sure a bug is filed to make this right, and reference that bug here.  Presumably we can get to the MediaElement from here somehow....

>+                  mozilla::dom::RequestContext::Audio,

Same.

>+++ b/dom/plugins/base/nsPluginHost.cpp
>+                       mozilla::dom::RequestContext::Plugin,

No need for "mozilla::" prefix.

>+                       mozilla::dom::RequestContext::Plugin,

And here.

>+++ b/dom/security/nsCORSListenerProxy.cpp
>+                       mozilla::dom::RequestContext::Internal,

No need for "mozilla::" prefix.

But also, per spec this should be the context of the channel we're doing the preflight for.  Please either fix or file a followup and reference it here.

>+++ b/dom/security/nsCSPContext.cpp
>+                         mozilla::dom::RequestContext::Cspreport);

No need for "mozilla::" prefix.

>+                         mozilla::dom::RequestContext::Cspreport);

And here.

>+++ b/dom/xbl/nsXBLResourceLoader.cpp
>+                                mozilla::dom::RequestContext::Image);

No need for "mozilla::" prefix.

>+++ b/dom/xbl/nsXBLService.cpp
>+                                              mozilla::dom::RequestContext::Internal,

No need for "mozilla::dom::" prefix.

>+                       mozilla::dom::RequestContext::Internal,

And here.

>+++ b/dom/xml/XMLDocument.cpp
>+                     mozilla::dom::RequestContext::Xmlhttprequest,

No need for "mozilla::dom::".

But also, this seems wrong.  Please raise a spec issue, bug on our side, point to it here, usual drill.

>+++ b/dom/xslt/base/txURIUtils.cpp
>+                                    mozilla::dom::RequestContext::Internal,

Are you sure?  Why not Xslt?  At the very least this needs a comment explaining why not Xslt.

Followup bug to sort this out, please.

>+++ b/dom/xslt/xslt/txMozillaStylesheetCompiler.cpp
>+                    mozilla::dom::RequestContext::Xslt,

No need for "mozilla::".

>+++ b/dom/xul/XULDocument.cpp
>+                           mozilla::dom::RequestContext::Internal,

No need for "mozilla::dom::".

>+                                mozilla::dom::RequestContext::Internal,

And here.

>+++ b/embedding/components/webbrowserpersist/nsWebBrowserPersist.cpp
>+                       mozilla::dom::RequestContext::Internal,

Why isn't this Download?  Needs at least a comment.

>+                       mozilla::dom::RequestContext::Internal);

Likewise.

>+++ b/gfx/thebes/gfxSVGGlyphs.cpp
>+                                  mozilla::dom::RequestContext::Internal,

No need for "mozilla::".

>+++ b/gfx/thebes/gfxUserFontSet.cpp
>+                                mozilla::dom::RequestContext::Internal))) {

No need for "mozilla::".  Also, why is this not Font?  Needs a comment.

>+++ b/image/decoders/icon/android/nsIconChannel.cpp
>+                                  mozilla::dom::RequestContext::Internal,

No need for "mozilla::".

>+++ b/image/public/imgILoader.idl

It might be more backwards-compatible to make the requestcontext arg to loadImageXPCOM optional and last, and default to "image" or whatever if not passed.  With the patch as-is you can get a value meant to be a contentpolicytype getting passed as requestcontext.

On the other hand, I see no addons in the addons mxr using this API, so maybe it's not a big deal...

>+++ b/image/src/imgLoader.cpp
>+    int index = FindEnumStringIndexImpl(aRequestContext.BeginReading(),
>+                                        aRequestContext.Length(),
>+                                        RequestContextValues::strings);

OK, this is somewhat sucky.

Can you file a followup on maybe putting an inline FromString() method (or two, one for nsAString, one for nsACString) in RequestContextValues so this could become:

  RequestContext context = RequestContextValues::FromString(aRequestContext);
  if (context == RequestContext::EndGuard_) {
      return NS_ERROR_INVALID_ARG;
  }

or some such?

>+++ b/intl/strres/nsStringBundle.cpp
>+                     mozilla::dom::RequestContext::Internal);

No need for "mozilla::".

>+++ b/js/xpconnect/loader/mozJSComponentLoader.cpp
>+                             mozilla::dom::RequestContext::Script,

No need for "mozilla::".

>+++ b/js/xpconnect/loader/mozJSSubScriptLoader.cpp
>+                       mozilla::dom::RequestContext::Internal,

No need for "mozilla::".

Also, why is this internal while component loader is script?  Seems to me like they should match.... followup bug for that is ok.

>+                                mozilla::dom::RequestContext::Internal);

And here.

>+++ b/js/xpconnect/src/XPCJSRuntime.cpp
>+                       mozilla::dom::RequestContext::Internal);

No need for "mozilla::".

>+++ b/layout/generic/nsImageFrame.cpp
>+                       mozilla::dom::RequestContext::Image,

No need for "mozilla::dom::".

>+++ b/layout/mathml/nsMathMLChar.cpp
>+                                                mozilla::dom::RequestContext::Internal);

No need for "mozilla::".

>+++ b/layout/style/FontFaceSet.cpp
>+                                            mozilla::dom::RequestContext::Font,

No need for "mozilla::dom::".

>+                                            mozilla::dom::RequestContext::Font);

And here.

>+++ b/layout/style/ImageLoader.cpp
>+                            mozilla::dom::RequestContext::Image);

No need for "mozilla::".

>+++ b/layout/style/Loader.cpp
>+                                                mozilla::dom::RequestContext::Internal);

No need for "mozilla::dom::".

>+                         mozilla::dom::RequestContext::Internal);

And here.

>+                                              mozilla::dom::RequestContext::Style,

And here.

>+                       mozilla::dom::RequestContext::Style,

And a fourth spot.

>+++ b/layout/xul/nsImageBoxFrame.cpp
>+                                mozilla::dom::RequestContext::Image);

No need for "mozilla::".

>+++ b/layout/xul/tree/nsTreeBodyFrame.cpp
>+                                                mozilla::dom::RequestContext::Image);

No need for "mozilla::".

>+++ b/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
>+  auto context = mozilla::dom::RequestContext::Internal;

No need for "mozilla::dom::".

>+++ b/netwerk/base/LoadInfo.cpp
>+                   mozilla::dom::RequestContext aContext,

Drop "mozilla::".

>+                   mozilla::dom::RequestContext aContext,

And here.

>+++ b/netwerk/base/LoadInfo.h
>+           mozilla::dom::RequestContext aContext,

Drop "mozilla::".

>+           mozilla::dom::RequestContext aContext,

And here.

>+  mozilla::dom::RequestContext mContext;

And here.

>+++ b/netwerk/base/nsIIOService.idl
>+    nsresult NewChannelFromURI2(nsIURI* aURI,

Document that this is not NS_IMETHOD because it's only usable from inside libxul anyway?

>+    nsresult NewChannel2(const nsACString& aSpec,

Likewise.

>+++ b/netwerk/protocol/http/HttpChannelChild.cpp
>+  openArgs.requestContext() = mozilla::dom::RequestContext::Internal;

No "mozilla::dom::".

>+++ b/netwerk/protocol/http/HttpChannelParent.cpp
>+                                 const mozilla::dom::RequestContext& aRequestContext,

No "mozilla::".

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8587682 [details] [diff] [review]
Part 2: Fix the JS callers of imgILoader.loadImageXPCOM

Canceling the other reviews here pending the sorting out of the desired API.

Comment 10

[Cameron Kaiser [:spectre]]

(In reply to Not doing reviews right now from comment #7)
> So the good news is that newChannel2/newChannelFromURI2 are not used by
> extensions yet.  Hopefully that will continue until we ship this.
> Unfortunately, we shipped those APIs already, in 36...

Is newChannel()/newChannelFromURI() being deprecated?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Is newChannel()/newChannelFromURI() being deprecated?

Not yet.

Comment 12

[(no longer active)]

Christoph, what are your plans for changing these call sites?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #12)
> Christoph, what are your plans for changing these call sites?

As discussed over email, Ehsan will join our weekly GECKO REVAMP meeting. I will post a short summary of the outcome here later this week.

Comment 14

[(no longer active)]

There's nothing left to be done here.