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

RESOLVED FIXED in mozilla36

Status

()

Core
DOM: Security
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

unspecified
mozilla36
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Updated

4 years ago
Blocks: 1006868
Depends on: 1038756
(Assignee)

Updated

4 years ago
Assignee: nobody → mozilla
(Assignee)

Updated

3 years ago
Blocks: 1076896
(Assignee)

Comment 1

3 years ago
Created attachment 8498893 [details] [diff] [review]
aboutmodule_newChannel2.patch

Similar to Bug 1067471, this patch just adds NewChannel2 and forwards all calls from newChannel to NewChannel2 using a nullptr-loadinfo.
Attachment #8498893 - Flags: review?(jonas)
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.
Attachment #8498893 - Flags: review?(jonas) → review-
(Assignee)

Updated

3 years ago
Blocks: 1087442
(Assignee)

Updated

3 years ago
Depends on: 1067471
(Assignee)

Comment 3

3 years ago
Created attachment 8509693 [details] [diff] [review]
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.
Attachment #8498893 - Attachment is obsolete: true
Attachment #8509693 - Flags: review?(jonas)
(Assignee)

Comment 4

3 years ago
Created attachment 8509695 [details] [diff] [review]
shim_1_aboutmodule_callers.patch
Attachment #8509695 - Flags: review?(jonas)
(Assignee)

Comment 5

3 years ago
Created attachment 8509970 [details] [diff] [review]
shim_1_aboutmodule.patch

Carrying over r+. Just forgot to rev the uuid in the idl.
Attachment #8509693 - Attachment is obsolete: true
Attachment #8509970 - Flags: review+
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/7497797d45c4
https://hg.mozilla.org/mozilla-central/rev/69a8e45a9777
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.