1232430 - [tracking bug] Convert JS callsites to use asyncOpen2 within dom/

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1232430_asyncopen2_dom.patch

We are about to convert all callsites of asyncOpen() to use asyncOpen2() which then performs security checks within asyncOpen2 - but before I do have quite so many questions:

@Jonas: Within dom/media/IdpSandbox.jsm we use systemPrincipal as the triggeringPrincipal even though we have a loadingPrincipal. That doesn't quite make sense to me. On the other hand I don't fully understand what that code is doing so I am also not sure if SEC_ALLOW_CROSS_ORIGIN_DATA_INHERITS is the right securityFlag, probably SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL is more appropriate.

@Dragana: Within dom/push/PushServiceHttp2.jsm, we use the channel as aContext as the second argument for asyncOpen(). Is there a better way to eliminate that second argument or should we just have some little helper 'mediator'-object that basically chains the listener and forwards the calls as we have done for other callsites?

@Aus: Within dom/browser-element/BrowserElementParent.js we might end up with a 'null' loadingPrincipal which is not allowed when calling asyncOpen2(). What is that code doing exactly? We need to find a fallback principal. Once we know what the code is doing in particular we can find the right fallbackPrincipal as well as securityFlag.

Thanks everyone for your input and help!

Comment 2

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

Sorry, I don't know what IdpSandbox.jsm does.

Might be worth breaking this bug up into several bugs though... Generally speaking we should have one "problem" per bug.

Comment 3

[Dragana Damjanovic [:dragana]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Created attachment 8698316 [details] [diff] [review]
> bug_1232430_asyncopen2_dom.patch
> 
> We are about to convert all callsites of asyncOpen() to use asyncOpen2()
> which then performs security checks within asyncOpen2 - but before I do have
> quite so many questions:
> 
> @Dragana: Within dom/push/PushServiceHttp2.jsm, we use the channel as
> aContext as the second argument for asyncOpen(). Is there a better way to
> eliminate that second argument or should we just have some little helper
> 'mediator'-object that basically chains the listener and forwards the calls
> as we have done for other callsites?
> 

It should be working if you set context to null. This has left from an old version, I just forgot to change it.

Comment 4

[Aus Lacroix [:aus]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> Created attachment 8698316 [details] [diff] [review]
> bug_1232430_asyncopen2_dom.patch
> 
> @Aus: Within dom/browser-element/BrowserElementParent.js we might end up
> with a 'null' loadingPrincipal which is not allowed when calling
> asyncOpen2(). What is that code doing exactly? We need to find a fallback
> principal. Once we know what the code is doing in particular we can find the
> right fallbackPrincipal as well as securityFlag.
> 
> Thanks everyone for your input and help!

The goal of grabbing the loadingPrincipal is to have access to the correct cookie jar and other session related information so that you can download content properly because, often, sites require sign-in which is verified via session data.

Hope that clarifies things. :)

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8698316 [details] [diff] [review]
bug_1232430_asyncopen2_dom.patch

Splitting this bug up into individual sub-bugs. Keeping this bug around as a tracking bug though.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

By now we are almost done with converting those callsites - unassigning myself from the tracking bug.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

All of blocking bugs have been resolved - time to close this one - hurray!