Christoph Kerschbaumer [:ckerschb] Reporter
	Description • 6 years ago

      No description provided.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 1 • 6 years ago

Hey Chris, we are currently applying a CSP to all of our content privileged about pages (See Meta Bug 1449872). Can you please connect us with the right folks working on about:newtab (activity stream). It seems currently about about:newtab pulls all the code from [1], right?

Currently we have an assertion that waves through content privileged about: pages that don't yet have a strong CSP applied and ultimately we would like to remove that assertion entirely so that all content privileged about pages need to ship with a CSP. Thanks!

[1] https://github.com/mozilla/activity-stream

	Chris Karlof [:ckarlof]
	Comment 2 • 6 years ago

Tim Spurway is the engineering manager for New Tab.

	Tim Spurway [:tspurway]
	Comment 3 • 6 years ago

Hey :ckerschb.  The activity stream code is currently pulled from [1] (from comment1).  

We have a current bug that is scheduled for Fx62 that is relevant here (bug 1448359), which, in effect would 'turn on' basic CSP for about:home and about:newtab.  

We have some ideas on the 'strong' CSP part and could set up a meeting to discuss.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 4 • 6 years ago

(In reply to Tim Spurway [:tspurway] from comment #3)
> Hey :ckerschb.  The activity stream code is currently pulled from [1] (from
> comment1).  
> 
> We have a current bug that is scheduled for Fx62 that is relevant here (bug
> 1448359), which, in effect would 'turn on' basic CSP for about:home and
> about:newtab.  
> 
> We have some ideas on the 'strong' CSP part and could set up a meeting to
> discuss.

Yeah, I a meeting sounds good. :jkt already left some notes here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1448359#c2

Probably we should move the discussion over to Bug 1448359 anyway.

	Vinothkumar Nagasayanan [:vinoth] Assignee
	Comment 6 • 6 years ago

Comment on attachment 8988468 [details]
Bug 1459544 - home,newtab,welcome removed from all.js

Please review the patch and let me know if changes are needed.

	Phabricator Automation
	Comment 7 • 6 years ago

Comment on attachment 8988468 [details]
Bug 1459544 - home,newtab,welcome removed from all.js

Christoph Kerschbaumer [:ckerschb] has approved the revision.

https://phabricator.services.mozilla.com/D1861

	Vinothkumar Nagasayanan [:vinoth] Assignee
	Comment 8 • 6 years ago

(In reply to Phabricator Automation from comment #7)
> Comment on attachment 8988468 [details]
> Bug 1459544 - home,newtab,welcome removed from all.js
> 
> Christoph Kerschbaumer [:ckerschb] has approved the revision.
> 
> https://phabricator.services.mozilla.com/D1861

Ya I will do a TRY push and check. also will fix the nit. Thanks.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 9 • 6 years ago

Comment on attachment 8988468 [details]
Bug 1459544 - home,newtab,welcome removed from all.js

This already has my r+; thanks!

	Vinothkumar Nagasayanan [:vinoth] Assignee
	Comment 10 • 6 years ago

Hey k88hudson,

In somecases about:newtab is loaded but the content of the newtab is a blank document.

Hence assertion failure happens when we try to check for CSP from META tags.

Few test files that trigger the assertion are,
1) browser_bug609700.js
2) browser_startup_images.js
3) browser_bug1025195_switchToTabHavingURI_aOpenParams.js
4) browser_remoteness_flip_on_restore.js

So I have two quick questions,

1) What are the cases where about:newtab contents are overridden with about:blank contents but still the URI is about:newtab ?
2) Apart from aboutNewTabServer.js any other file implements the overriding behaviour ?

Please let me know incase you know any details about this.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 11 • 6 years ago

It seems we should only assert in case nothing stopped the document load on purpose. Probably we can use mParserAborted to realize that.

Seems to work (at least locally):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c40b9f364b08dde54f21196569b0548475d9fe7

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 12 • 6 years ago

If that turns out to work then we need to write a test where we register a new about page that does *not* have a meta CSP attached and then annotate that test with expectedAssertions 1 or some similar test to verify our assertion keeps working correctly.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 13 • 6 years ago

Hey Gijs,

when you factored out the about page registration and added BrowserTestUtils.registerAboutPage() [1] did browser_e10s_about_page_triggeringprincipal.js still work correctly?

I am about to add a test making sure that we assert in case a new registered about page does not ship with a CSP. I run the attached test and get the following error (see stacktrace underneath). I get the same error when running browser_e10s_about_page_triggeringprincipal.js. It seems the problem is related to the flag Ci.nsIAboutModule.URI_MUST_LOAD_IN_CHILD. If I remove that, then for now at least my test runs to completion and doesn't throw the NS_ERROR_MALFORMED_URI exception.

Seems surprising to me that browser_e10s_about_page_triggeringprincipal.js works on TRY given that it doesn't locally. Any idea what might be wrong here? Or am I just missing something?

[1] https://hg.mozilla.org/mozilla-central/rev/38740bd3a50e#l2.21


 0:27.04 GECKO(26695) [Child 26878, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file /home/ckerschb/source/mc/netwerk/base/nsNetUtil.cpp, line 235
 0:27.11 INFO Console message: [JavaScript Error: "[Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIWebNavigation.loadURIWithOptions]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: resource://gre/actors/WebNavigationChild.jsm :: loadURI/< :: line 112"  data: no]"]
loadURI/<@resource://gre/actors/WebNavigationChild.jsm:112:14
_wrapURIChangeCall@resource://gre/actors/WebNavigationChild.jsm:63:7
loadURI@resource://gre/actors/WebNavigationChild.jsm:111:5
receiveMessage@resource://gre/actors/WebNavigationChild.jsm:43:9
receiveMessage@resource://gre/modules/ActorManagerChild.jsm:91:39
MessageListener.receiveMessage*init@resource://gre/modules/ActorManagerChild.jsm:40:7
attach@resource://gre/modules/ActorManagerChild.jsm:229:48
@chrome://global/content/browser-content.js:14:1

	:Gijs (he/him)
	Comment 14 • 6 years ago

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> Created attachment 9007994 [details] [diff] [review]
> about_page_test.patch
> 
> Hey Gijs,
> 
> when you factored out the about page registration and added
> BrowserTestUtils.registerAboutPage() [1] did
> browser_e10s_about_page_triggeringprincipal.js still work correctly?

It was 18 months ago, I really don't recall, unfortunately. I assume so... from looking at it very very quickly, I imagine that the issue is with the fact that we're trying to load the content script for the registrations on-demand, and then immediately send a message. Presumably the load of the script races with the message and thus we never register the about: page in the child.

One possible way around this would be to move the child-process registering thing to a content process script that we always load earlier, e.g. content-utils.js . An easy way to see if that is indeed the issue would be to just cut/paste the contents of  mozilla-central/testing/mochitest/BrowserTestUtils/content/content-about-page-utils.js into  content-utils.js and see if that fixes the issue for you locally.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 15 • 6 years ago

Comment on attachment 9007994 [details] [diff] [review]
about_page_test.patch

(In reply to :Gijs (he/him) from comment #14)
> One possible way around this would be to move the child-process registering
> thing to a content process script that we always load earlier, e.g.
> content-utils.js . An easy way to see if that is indeed the issue would be
> to just cut/paste the contents of 
> mozilla-central/testing/mochitest/BrowserTestUtils/content/content-about-
> page-utils.js into  content-utils.js and see if that fixes the issue for you
> locally.

Thanks Gijs, but your idea did not fix the problem. I didn't manage to receive a message within content-utils.js for registering the about page. We have to dig a little deeper to find out where the problem is. I filed Bug 1490977 to write that test and to figure the problem.

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 16 • 6 years ago

Smaug, as discussed the other day it makes more sense to only assert that an about page has indeed a CSP applied if the document load was not cancelled. Thanks!

	Christoph Kerschbaumer [:ckerschb] Reporter
	Comment 17 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b3b4e1622a08e20ccdcaa3e7d32924fb6252d76

	Pulsebot
	Comment 18 • 6 years ago

Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/084a50d2778a
Only assert that about page has CSP if nothing stopped the load of the doc. r=smaug

	Noemi Erli[:noemi_erli]
	Comment 19 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/084a50d2778a