1067468 - Add newChannel2 to nsIAboutModule so we can attach a LoadInfo to the created channel

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

We should add newChannel2() with the following interface to nsIAboutModule;

> nsIChannel newChannel2(in nsIURI aURI,
>                         in nsIDOMDocument aRequestingDocument,
>                         in nsIPrincipal aRequestingPrincipal,
>                         in unsigned long aSecurityFlags,
>                         in unsigned long aContentPolicyType);

Once that interface is landed we can start migrating consumers to use newChannel2() rather than newChannel() to create a channel.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: aboutmodule_newChannel2.patch

Similar to Bug 1067471, this patch just adds NewChannel2 and forwards all calls from newChannel to NewChannel2 using a nullptr-loadinfo.

Comment 2

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

Comment on attachment 8498893 [details] [diff] [review]
aboutmodule_newChannel2.patch

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

Is it really worth messing with nsIAboutModule at this point? We can add the loadinfo in nsAboutProtocolHandler::NewChannel2, and we can do security checks in nsAboutProtocolHandler::AsyncOpen2 without having to touch nsIAboutModule.

The only advantage to adding newChannel2 to nsIAboutModule is that it would allow about modules inspect the loadinfo and change the behavior of newChannel in response to it. But we don't have the need for that yet.

In order for any of this code to be used we'll also have to add code to nsAboutProtocolHandler to attempt to call nsIAboutModule.newChannel2, and then fall back to nsIAboutModule.newChannel as needed. And possibly add wrappers if we actually want the various about modules to take responsibility for doing security checks.

Also, I think we can simply add another argument to nsIAboutModule.newChannel. No need to introduce a separate nsIAboutModule.newChannel2. JS implementations will simply ignore the extra argument. And we're not relying on them to do anything important anyway.

In general, it seems simpler to land this stuff after we have more of the other plumbing in place IMHO. But up to you. But if so, don't add newChannel2.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: shim_1_aboutmodule.patch

(In reply to Jonas Sicking (:sicking) from comment #2)
> Comment on attachment 8498893 [details] [diff] [review]
> aboutmodule_newChannel2.patch
> 
> Review of attachment 8498893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is it really worth messing with nsIAboutModule at this point? We can add the
> loadinfo in nsAboutProtocolHandler::NewChannel2, and we can do security
> checks in nsAboutProtocolHandler::AsyncOpen2 without having to touch
> nsIAboutModule.

We could add the loadInfo in nsAboutProtocolHandler::NewChannel2, but since we are already having a ::NewChannel2 in nsAboutProtocolhandler, why not pass the loadInfo along to nsIAboutModule? The reason I prefer this design is that we (at some point, see bug 1087442) attach the loadInfo at creation time of a channel and immediately can inspect the loadinfo.

> The only advantage to adding newChannel2 to nsIAboutModule is that it would
> allow about modules inspect the loadinfo and change the behavior of
> newChannel in response to it. But we don't have the need for that yet.

Agreed, not yet, but soon :-)

> Also, I think we can simply add another argument to
> nsIAboutModule.newChannel. No need to introduce a separate
> nsIAboutModule.newChannel2. JS implementations will simply ignore the extra
> argument. And we're not relying on them to do anything important anyway.
> 
> In general, it seems simpler to land this stuff after we have more of the
> other plumbing in place IMHO. But up to you. But if so, don't add
> newChannel2.

Agreed, we can simply extend NewChannel with a loadInfo argument. Once that patch has landed, we can attach the loadInfo to the channel in bug 1087442.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: shim_1_aboutmodule_callers.patch

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: shim_1_aboutmodule.patch

Carrying over r+. Just forgot to rev the uuid in the idl.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7497797d45c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/69a8e45a9777

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/7497797d45c4
https://hg.mozilla.org/mozilla-central/rev/69a8e45a9777