1967731 - VideoDocument starts loading stylsheets/scripts before ClientSource (and thus the ClientInfo) is initialized.

Status: RESOLVED FIXED

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

Description

[Tom Schuster]

When loading a VideoDocument, we will start loading stylesheets and scripts from VideoDocument::SetScriptGlobalObject. This function is called from nsGlobalWindowOuter::SetNewDocument. However SetNewDocument calls nsGlobalWindowInner::ExecutionReady (which calls EnsureClientSource()) only afterwards. That means we start loading before the document's ClientSource and thus ClientInfo are initialized!

Is there some other point afterwards that we could use to start loading?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

The current setup seems to be there on purpose https://bugzilla.mozilla.org/show_bug.cgi?id=708431
But extensions shouldn't use nsIContentPolicy directly anymore.

Comment 2

[Tom Schuster]

I think we do have to worry about page's CSP blocking the script when using a data: or blob: iframe, which would inherit the CSP.
Edit: We can probably just special case these URLs in SubjectToCSP. I am looking.

Comment 3

[Tom Schuster]

I don't think this warrants sec-moderate by itself. We were purposefully bypassing some security checks, because we don't want the load of these internal sheets and scripts to be blocked (by e.g. extensions or a page's CSP). I mostly filed this as a security bug, because this work blocks bug 1966927 and I didn't want to accidentally leak something.

Comment 4

[Tom Schuster]

Attachment: Bug 1967731 - Simplify loading in VideoDocument. r?smaug

Comment 5

[Tom Schuster]

Attachment: Bug 1967731 - Test that TopLevelVideoDocument.js loads even with a strict CSP. r?smaug

Without the exemption added to SubjectToCSP the existing test test_videocontrols_standalone.html would fail with this CSP.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7082569b9321
Simplify loading in VideoDocument. r=smaug

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7082569b9321

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to Tom Schuster (MoCo) from comment #3)

I don't think this warrants sec-moderate by itself. We were purposefully bypassing some security checks, because we don't want the load of these internal sheets and scripts to be blocked

Thanks. When I rated it I didn't realize it was only a couple of hardcoded internal resources. Those are going to be exempt from being blocked by CSP anyway so nothing is being bypassed.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:tschuster, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox140 to wontfix.

For more information, please visit BugBot documentation.

Comment 10

[Tom Schuster]

Comment on attachment 9490002 [details]
Bug 1967731 - Simplify loading in VideoDocument. r?smaug

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency:
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:
• Is Android affected?: Yes

Comment 11

[Tom Schuster]

Required for bug 1966927.

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9490002 [details]
Bug 1967731 - Simplify loading in VideoDocument. r?smaug

Approved for 140.0b7

Comment 13

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/e415d9d15705
https://hg.mozilla.org/releases/mozilla-beta/rev/038ac50dc894

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6de826a6299b
https://hg.mozilla.org/integration/autoland/rev/9afb54e8885f
Test that TopLevelVideoDocument.js loads even with a strict CSP. r=smaug

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9afb54e8885f