1797843 - Add support to play streaming video inside new user onboarding

Status: VERIFIED FIXED

(Firefox :: Messaging System, task, P1)

Description

[Punam Dahiya [:pdahiya]]

Scope of this bug is to implement support to play streaming video inside new user onboarding.

we can use https://developer.mozilla.org/en-US/docs/Web/HTML/Element/video or investigate any other API already available in m-c

Link of video to use https://drive.google.com/file/d/1RnSrhpAtb-kT5PfZL2M_LEmPAokZbNBM/view?usp=sharing

We should also look into possibility of hosting video on RemoteSettings

Comment 1

[yozhang]

Attachment: Bug 1797843 - Add support to play streaming video inside new user onboarding r=negin,pdahiya,emcminn,aminomancer

Comment 2

[Punam Dahiya [:pdahiya]]

Attached patch is still failing on try server even after adding default-src as chrome:; in about:welcome CSP declaration

https://hg.mozilla.org/try/rev/b99ce35697dcf702b7c1139df6f4c7eb8a307a4f#l2.13

Error:
https://treeherder.mozilla.org/logviewer?job_id=396238988&repo=try&lineNumber=12707

Link to try run
https://treeherder.mozilla.org/jobs?repo=try&revision=a1f0200d254a7aa8818ff8161ffd34b15c62b0e2

Failure is only seen for debug builds in all OS
https://treeherder.mozilla.org/push-health/push?repo=try&revision=a1f0200d254a7aa8818ff8161ffd34b15c62b0e2&testGroup=pr&selectedTest=browsercomponentsnewtabtestbrowserbrowseraboutwelcomemultistagemrjs&selectedTaskId=

NI @ckerschb for feedback on why AssertAboutPageHasCSP failing for attached patch thanks!

Comment 3

[Ed Lee :Mardak]

The assertion is triggering when running test_aboutwelcome_video_content:
https://searchfox.org/mozilla-central/rev/dd216c1307a2bf1b0d2465b9749fa86dac44303a/browser/components/newtab/test/browser/browser_aboutwelcome_multistage_mr.js#293-312

It ends up rendering:

<video controls="" src="" value="video_container" width="604px" height="340px"><source src=""></video>

It's not the about:welcome page's CSP that is problematic as it doesn't assert for the other tests that load about:welcome.

Comment 4

[Ed Lee :Mardak]

Attachment: non-debug build running test_aboutwelcome_video_content

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b4e0751850a
Add support to play streaming video inside new user onboarding r=pdahiya

Comment 6

[Cristian Tuns]

Backed out for causing mochitest failures on nsContentSecurityUtils.cpp

• Backout link
• Push with failures
• Failure Log
• Failure line: Assertion failure: foundDefaultSrc (about: page must contain a CSP including default-src), at /builds/worker/checkouts/gecko/dom/security/nsContentSecurityUtils.cpp:1238

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Punam Dahiya [:pdahiya] from comment #2)

NI @ckerschb for feedback on why AssertAboutPageHasCSP failing for attached patch thanks!

Ultimately we want to make sure that every about: page has as strong of CSP as possible.

To locate this problem and to make sure we have the best possible CSP I think it would be good to know what
(a) about: page exactly is causing the error, and
(b) what the CSP is.

I guess it's simplest to print the documentURI and the parsedPolicyStr from within AssertAboutPageHasCSP

Comment 8

[Shane Hughes [:aminomancer]]

(In reply to Ed Lee :Mardak from comment #3)

The assertion is triggering when running test_aboutwelcome_video_content:
https://searchfox.org/mozilla-central/rev/dd216c1307a2bf1b0d2465b9749fa86dac44303a/browser/components/newtab/test/browser/browser_aboutwelcome_multistage_mr.js#293-312

It ends up rendering:

<video controls="" src="" value="video_container" width="604px" height="340px"><source src=""></video>

It's not the about:welcome page's CSP that is problematic as it doesn't assert for the other tests that load about:welcome.

That markup is correct, the test just explicitly sets video_url: "" since I guess we don't have any video files on train or hosted somewhere that wouldn't crash the test harness? I see what you mean about other tests, but it's hard to see why this particular assertion would throw the test if the issue wasn't related to the CSP. And although the empty video display looks like an error message, it can't throw a test as far as I can tell. Just console warnings:

HTTP “Content-Type” of “text/html” is not supported. Load of media resource failed.
Cannot play media. No decoders for requested formats: text/html

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)

To locate this problem and to make sure we have the best possible CSP I think it would be good to know what
(a) about: page exactly is causing the error, and
(b) what the CSP is.

The page is about:welcome, and here is the CSP. The patch is basically trying to render a video in it from any of the origins registered for media-src.

Comment 9

[Punam Dahiya [:pdahiya]]

@ckerschb please see below for about:welcome CSP used in patch when failure is seen , do you now why this CSP failure is debug build specific ? Thanks

"default-src chrome:; object-src 'none'; script-src resource: chrome:; media-src resource: chrome: https://assets.mozilla.net https://www.mozilla.org; connect-src https:; img-src https: data: blob: chrome:; style-src resource: chrome:;"

Comment 10

[Shane Hughes [:aminomancer]]

(In reply to Punam Dahiya [:pdahiya] from comment #9)

@ckerschb please see below for about:welcome CSP used in patch when failure is seen , do you now why this CSP failure is debug build specific ? Thanks

That's just bc the whole AssertAboutPageHasCSP assertion is only run for debug builds. I'm not sure what the video has to do with it though

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Punam Dahiya [:pdahiya] from comment #9)

@ckerschb please see below for about:welcome CSP used in patch when failure is seen , do you now why this CSP failure is debug build specific ? Thanks

"default-src chrome:; object-src 'none'; script-src resource: chrome:; media-src resource: chrome: https://assets.mozilla.net https://www.mozilla.org; connect-src https:; img-src https: data: blob: chrome:; style-src resource: chrome:;"

Something sounds wrong here, but let's start at the beginning: We added that assertion to make sure no new about: page can be added without a CSP that does not have strong security properties. For example, this assertion ensures that about:welcome has a CSP that at least includes a default-src directive.

When looking at the CSP you are posting and also when looking at your patch, I see there is a CSP with a valid default-src directive. So the assertion should not trigger. FWIW, it's only needed for debug builds since we wanted some kind of reporting mechansim that we can not accidentally forget to add a CSP for a newly added about page.

So, to me the assertion should not trigger at all. I am sure you tested this carefully, but could it be possible that we are loading some other about page in between or something?

If it's too hard to figure out I can offer to apply the patch tomorrow and run it on my machine locally to get a sense of what is happening.

Comment 12

[Ed Lee :Mardak]

mconley, do you know if there have been issues loading videocontrols from about: pages? I think its new DOMParser().parseFromString behavior is triggering AssertAboutPageHasCSP debug checks?

https://searchfox.org/mozilla-central/rev/eadcd17181cdc44ea5b939ad506edd79d1270872/toolkit/content/widgets/videocontrols.js#2841-2842

Comment 13

[Ed Lee :Mardak]

ckerschb, running a debug build opening about:welcome then doing this from the web console:

new DOMParser().parseFromString("", "application/xml")

does indeed crash the tab:

Gah. Your tab just crashed.
We can help!

Choose Restore This Tab to reload the page.

Assertion failure: foundDefaultSrc (about: page must contain a CSP including default-src), at /builds/worker/checkouts/gecko/dom/security/nsContentSecurityUtils.cpp:1238

Vs doing so from some webpage:

XML Parsing Error: no root element found
Location: https://bugzilla.mozilla.org/home
Line Number 1, Column 1: home:1:1
XMLDocument { … }

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b4b31222419
Add support to play streaming video inside new user onboarding r=pdahiya

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ed Lee :Mardak from comment #13)

ckerschb, running a debug build opening about:welcome then doing this from the web console:

new DOMParser().parseFromString("", "application/xml")

Assertion failure: foundDefaultSrc (about: page must contain a CSP including default-src), at /builds/worker/checkouts/gecko/dom/security/nsContentSecurityUtils.cpp:1238

Mhm, does not crash on my end when I open about:welcome and then paste your code into the console. I also tried running test browser_aboutwelcome_multistage_mr.js adding all sorts of debug information - does not trigger the assertion. We are opening about:welcome which has a valid CSP including a default-src directive. Not sure what I am doing wrong. Anything else I am missing?

Comment 16

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/2b4b31222419

Comment 17

[Ed Lee :Mardak]

Hmm… my mozconfig has these 2 entries:

ac_add_options --enable-debug
ac_add_options --enable-artifact-builds

And I just checked latest m-c https://hg.mozilla.org/mozilla-central/rev/8495494c57f82b71854b362bfbd40f9998933908

The debug build crashes the about:welcome tab with just new DOMParser().parseFromString("", "application/xml") evaluated in the console resulting in:

Assertion failure: foundDefaultSrc (about: page must contain a CSP including default-src), at /builds/worker/checkouts/gecko/dom/security/nsContentSecurityUtils.cpp:1238
#01: nsContentSecurityUtils::AssertAboutPageHasCSP(mozilla::dom::Document*)[NightlyDebug.app/Contents/MacOS/XUL +0x3d3e563]
#02: mozilla::dom::Document::EndLoad()[NightlyDebug.app/Contents/MacOS/XUL +0x1765bce]
#03: mozilla::dom::XMLDocument::EndLoad()[NightlyDebug.app/Contents/MacOS/XUL +0x4394748]
#04: nsXMLContentSink::DidBuildModel(bool)[NightlyDebug.app/Contents/MacOS/XUL +0x4397e62]
#05: nsParser::DidBuildModel()[NightlyDebug.app/Contents/MacOS/XUL +0xf89742]
#06: nsParser::Terminate()[NightlyDebug.app/Contents/MacOS/XUL +0xf898de]
#07: nsParser::ResumeParse(bool, bool, bool)[NightlyDebug.app/Contents/MacOS/XUL +0xf89ecb]
#08: nsParser::OnStopRequest(nsIRequest*, nsresult)[NightlyDebug.app/Contents/MacOS/XUL +0xf8ba58]
#09: mozilla::dom::DOMParser::ParseFromStream(nsIInputStream*, nsTSubstring<char16_t> const&, int, mozilla::dom::SupportedType, mozilla::ErrorResult&)[NightlyDebug.app/Contents/MacOS/XUL +0x16e7526]
#10: mozilla::dom::DOMParser::ParseFromString(nsTSubstring<char16_t> const&, mozilla::dom::SupportedType, mozilla::ErrorResult&)[NightlyDebug.app/Contents/MacOS/XUL +0x16e67e5]
#11: mozilla::dom::DOMParser_Binding::parseFromString(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&)[NightlyDebug.app/Contents/MacOS/XUL +0x28a4166]
…

The call stack does seem to match up with the earlier debug try running browser_aboutwelcome_multistage_mr.js https://treeherder.mozilla.org/logviewer?job_id=396238988&repo=try&lineNumber=12707

If running with MOZ_LOG=DocumentLeak:5,CSP:5, the lines immediately before the assert stack (after letting about:welcome load, opening devtools and pasting in the code)

[Child 1338: Main Thread]: D/DocumentLeak DOCUMENT 11356b200 created
[Child 1338: Main Thread]: D/DocumentLeak DOCUMENT 11356b200 StartDocumentLoad about:welcome
[Child 1338: Main Thread]: D/CSP CSPService::ShouldLoad called for chrome://global/locale/layout/xmlparser.properties
Assertion failure: foundDefaultSrc (about: page must contain a CSP including default-src), at /builds/worker/checkouts/gecko/dom/security/nsContentSecurityUtils.cpp:1238

Applying the latest version of the patch that just merged to m-c, the test has been moved to browser_aboutwelcome_multistage_video.js and skipped for debug. Here's running with MOZ_LOG=DocumentLeak:5,CSP:5 ./mach test multistage_video

 0:21.52 GECKO(3018) [Child 3032: Main Thread]: D/CSP CSPService::ShouldLoad called for chrome://global/content/elements/videocontrols.js
 0:21.53 GECKO(3018) [Child 3032: Main Thread]: D/DocumentLeak DOCUMENT 111590d00 created
 0:21.53 GECKO(3018) [Child 3032: Main Thread]: D/DocumentLeak DOCUMENT 111590d00 StartDocumentLoad about:welcome
 0:21.53 GECKO(3018) Assertion failure: foundDefaultSrc (about: page must contain a CSP including default-src), at /builds/worker/checkouts/gecko/dom/security/nsContentSecurityUtils.cpp:1238

I'll try to see if someone else can reproduce it this way with the manually evaluated parseFromString code.

Comment 18

[Mike Conley (:mconley) (:⚙️)]

(In reply to Ed Lee :Mardak from comment #12)

mconley, do you know if there have been issues loading videocontrols from about: pages? I think its new DOMParser().parseFromString behavior is triggering AssertAboutPageHasCSP debug checks?

https://searchfox.org/mozilla-central/rev/eadcd17181cdc44ea5b939ad506edd79d1270872/toolkit/content/widgets/videocontrols.js#2841-2842

Hm - the last time I recall us putting a video in an about: page was wayyyy back in the day in about:addons, and this was probably before the CSP protections (and definitely before UAWidgets).

We might want to involve some DOM people here to figure out what the right move is for this. Maybe smaug?

Comment 19

[Punam Dahiya [:pdahiya]]

I'll try to see if someone else can reproduce it this way with the manually evaluated parseFromString code.

I am able to replicate the crash in debug builds on latest central, here are the findings

1. Debug build with new DOMParser().parseFromString("", "application/xml") in web console crashes tab with assert failure in terminal

Assertion failure: foundDefaultSrc (about: page must contain a CSP including default-src), at /builds/worker/checkouts/gecko/dom/security/nsContentSecurityUtils.cpp:1238 ...

2. Non-debug artifact build with new DOMParser().parseFromString("", "application/xml") in web console , doesn't crash tab and throws below error in web console

XML Parsing Error: no root element found Location: about:welcome Line Number 1, Column 1:...

Attaching screenshots of above scenarios

Comment 20

[Punam Dahiya [:pdahiya]]

Attachment: XML Error on non-debug builds on about:welcome

Comment 21

[Punam Dahiya [:pdahiya]]

Attachment: Tab Crash with assert failiure on debug build

Comment 22

[nbulous]

This is needed for the Onboarding welcome video experiment in Fx108.

Comment 23

[Simona Rosu [:srosu], Ecosystem QA]

Attachment: FirefoxVideoNotDisplayedOnLinux.png

I‘ve verified this task using the latest Firefox Nightly 109.0a1 (Build ID: 20221115214157) on Windows 10 x64, macOS 11.7.1, and Ubuntu 22.04 x64.

• After following the steps from the Test Plan, I can confirm the followings:
  • On Windows 10 and macOS 11.7.1, the Firefox video shows on the “Thanks for downloading Firefox” screen of the “about:welcome” page. After the video ends, the “Gratitude” screen is displayed.
  • On Ubuntu 22.04, the Firefox Video is not displayed at all. Considering this, @Punam do you want to reopen this issue? Or should we file a new issue for Linux distros?

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Punam Dahiya [:pdahiya] from comment #19)

1. Debug build with new DOMParser().parseFromString("", "application/xml") in web console crashes tab with assert failure in terminal

Assertion failure: foundDefaultSrc (about: page must contain a CSP including default-src), at /builds/worker/checkouts/gecko/dom/security/nsContentSecurityUtils.cpp:1238 ...

I can reproduce the problem now:

• Set up a debug build
• Visit about:welcome
• Open the web console and paste new DOMParser().parseFromString("", "application/xml")

which then causes
Assertion failure: foundDefaultSrc (about: page must contain a CSP including default-src)

I suppose we are reusing the documentURI which is still about:welcome. Question is (most likely for smaug), can we somehow query that case within AssertAboutPageHasCSP where we have a Document* available and return; instead of causing the assertion to trigger?

Comment 25

[Punam Dahiya [:pdahiya]]

- On Ubuntu 22.04, the Firefox Video is not displayed at all. Considering this, @Punam do you want to reopen this issue? Or should we file a new issue for Linux distros?

New user onboarding video experiments are Windows only and looks good. Please file a bug to help track needed work for Linux for onboarding video feature. Thanks!

Comment 26

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

(I'm a bit lost here. The bug is closed, but some needinfo is still requested?)

ckerschb, do you mean the Document being the one created by DOMParser and then we assert something on it?
And you'd like to get the real page here https://searchfox.org/mozilla-central/rev/d7d2cc647772de15c4c5aa47f74d25d0e379e404/dom/security/nsContentSecurityUtils.cpp#1159 ?
One should be able to QI the return value of https://searchfox.org/mozilla-central/rev/d7d2cc647772de15c4c5aa47f74d25d0e379e404/dom/base/Document.h#1807 to nsPIDOMWindowInner and access GetExtantDoc() on it and access CSP from it.
Null check needed for the return value of all the methods starting with Get*

Comment 27

[Ed Lee :Mardak]

We could move the discussion to bug 1800535 as this bug has landed with a skip-if=debug to avoid the assertion.

Comment 28

[Simona Rosu [:srosu], Ecosystem QA]

Thanks for the reply, Punam! Filed a new issue for Linux distros: Bug 1801038.
Based on comment 23 and comment 25 above, I'm marking this task as Verified Fixed.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #26)

ckerschb, do you mean the Document being the one created by DOMParser and then we assert something on it?

Thanks Smaug for your pointers.

The reason the bug is closed is because skip-if debug is used for the test and AssertAboutPageHasCSP only triggers for debug builds.
From a Dom:Security point of view I filed Bug 1801063 which will query the ExtantDoc to avoid the assertion which then should allow us to remove the skip-if debug for the video tests within Bug 1800535.

Comment 30

[yozhang]

Comment on attachment 9301909 [details]
Bug 1797843 - Add support to play streaming video inside new user onboarding r=negin,pdahiya,emcminn,aminomancer

Beta/Release Uplift Approval Request

• User impact if declined: Experiment First Run stopped
• 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: See patch for steps: https://phabricator.services.mozilla.com/D161233
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Only includes architectural changes to support new types of messages, but doesn't actually add or modify any messages. so there aren't any user-facing changes in this patch, though there will be other patches that add/update messages in the future
• String changes made/needed: None
• Is Android affected?: No

Comment 31

[Dianna Smith [:diannaS]]

:Hi yozhang! Can I get a little more information on the risk level of the patch as well as the user impact? I know this is needed for Onboarding Video for 108, but if you can add that information, it would really be helpful. thank you!

Comment 32

[yozhang]

(In reply to Dianna Smith [:diannaS] from comment #31)

:Hi yozhang! Can I get a little more information on the risk level of the patch as well as the user impact? I know this is needed for Onboarding Video for 108, but if you can add that information, it would really be helpful. thank you!

Oops sorry, I seemed to have forgotten some details, I'll update that right now!

Comment 33

[Dianna Smith [:diannaS]]

Comment on attachment 9301909 [details]
Bug 1797843 - Add support to play streaming video inside new user onboarding r=negin,pdahiya,emcminn,aminomancer

Approved for 108.0b4

Comment 34

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f83eea8f7599

Comment 35

[Simona Rosu [:srosu], Ecosystem QA]

Hi @Punam! I've tried to verify this task today using the steps described on the provided Test Plan, and the "Welcome video" screen remains displayed when the video ends instead of redirecting to the "Gratitude" screen.

However, this behavior is not reproducible under the following circumstances:

• Using the latest version of Firefox Nightly while being enrolled in the "Onboarding video" experiment
• Using the steps from the above Test Plan with a Firefox Nightly build from 11/16/2022.

Considering the above, could you please tell us if the steps from the Test Plan are still up to date or if we should log an issue for this?

Comment 36

[Punam Dahiya [:pdahiya]]

Hi Simona, with telemetry fix Bug 1797851 in Nightly and 108 beta, test steps in the attached patch are outdated. NI Yoen to update video screen in Test Plan of attached patch with browser.aboutwelcome.screens value from https://phabricator.services.mozilla.com/D161704 . Thanks!

Comment 37

[Simona Rosu [:srosu], Ecosystem QA]

I‘ve verified this task using Firefox Beta 108.0b4 (Build ID: 20221120185746) on Windows 10 x64, macOS 11.7.1. On Ubuntu 22.04, I’m blocked from verifying due to Bug 1801038.

• After following the steps from this Test Plan, I can confirm the followings:
  • On Windows 10 and macOS 11.7.1, the Firefox video shows on the “Thanks for downloading Firefox” screen of the “about:welcome” page. After the video ends, the new tab page is displayed.