[:fabrice] Fabrice Desré Assignee
	Description • 11 years ago

Since we don't let 3rd parties change this CSP we can hardcode the check until the full c++ rewrite is done.

	Sid Stamm [:geekboy or :sstamm]
	Comment 2 • 11 years ago

Comment on attachment 817937 [details] [diff] [review]
patch

Review of attachment 817937 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Double check about the userPass and consider the rest of my comments as suggestions.

::: content/base/src/nsCSPService.cpp
@@ +105,5 @@
>      aContentType == nsIContentPolicy::TYPE_DOCUMENT) {
>      return NS_OK;
>    }
>  
> +  // ----- THIS IS A TEMPORARY FAST PATH FOR CERTIFIED APPS. -----

Please make a note here that this will be removed when we fix bug 925004.

@@ +125,5 @@
> +    // - never loading objects.
> +    // - accepting everything else.
> +
> +    nsAutoCString sourceOrigin;
> +    aRequestOrigin->GetPrePath(sourceOrigin);

In other bits of CSP we have to purge the userPass out of the URI.  I assume there's no userPass for certified apps, but I want to mention this in case there could be (and we have to strip them out).

@@ +133,5 @@
> +        !sourceOrigin.Equals(contentOrigin)) {
> +      *aDecision = nsIContentPolicy::REJECT_SERVER;
> +    } else if (nsIContentPolicy::TYPE_OBJECT == aContentType) {
> +      *aDecision = nsIContentPolicy::REJECT_SERVER;
> +    }

I think the allow/reject decisions would be easier to read as a switch, and could line up exactly with the comment above:

switch (aContentType) {
  case nsIContentPolicy::TYPE_SCRIPT:
  case nsIContentPolicy::TYPE_STYLESHEET:
    if (!sourceOrigin.Equals(contentOrigin)) {
      *aDecision = nsIContentPolicy::REJECT_SERVER;
    }
    break;

  case nsIContentPolicy::TYPE_OBJECT:
    *aDecision = nsIContentPolicy::REJECT_SERVER;
    break;

  default:
    *aDecision = nsIContentPolicy::ACCEPT;
}

And a minor optimization would be to extract the origins and do the compare all inside the relevant cases so the strings are only built if they'll be used.

	[:fabrice] Fabrice Desré Assignee
	Comment 3 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/92ad60388975

	Mike Lee [:mlee]
	Comment 4 • 11 years ago

+ koi+ per conversations with Vivien & Fabrice
+ Updated dependency relationship with bug 925004 per conversation with Fabrice.

	[:fabrice] Fabrice Desré Assignee
	Comment 5 • 11 years ago

Backed out in https://hg.mozilla.org/integration/b2g-inbound/rev/6b269d35936a

Try run to check the fix: (local tests are green)
https://tbpl.mozilla.org/?tree=Try&rev=adc20e052e5d

	[:fabrice] Fabrice Desré Assignee
	Comment 6 • 11 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/ac2eb4f684ad

	Carsten Book [:Tomcat]
	Comment 7 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/ac2eb4f684ad

	Ryan VanderMeulen [:RyanVM]
	Comment 8 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/56ec10275771

	Ben Kelly [:bkelly, not reviewing]
	Comment 9 • 11 years ago

I think this might have caused some bustage.  See the gaia unit test results here:

  https://tbpl.mozilla.org/?showall=1&tree=B2g-Inbound&rev=ac2eb4f684ad

And the first m-c run after this that includes gaia unit tests:

  https://tbpl.mozilla.org/?showall=1&rev=4e7d1e2c93a6

	Sid Stamm [:geekboy or :sstamm]
	Comment 10 • 11 years ago

I'm confused, the B2G test bustage is file not found errors during setup, and the second link you posted has all B2G tests as green.  

Am I missing something obvious?

https://tbpl.mozilla.org/php/getParsedLog.php?id=29283989&tree=B2g-Inbound
(see also bug 928147)

	Ben Kelly [:bkelly, not reviewing]
	Comment 11 • 11 years ago

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #10)
> I'm confused, the B2G test bustage is file not found errors during setup,
> and the second link you posted has all B2G tests as green.  
> 
> Am I missing something obvious?

See the red G on Linux 64opt line.

	Sid Stamm [:geekboy or :sstamm]
	Comment 12 • 11 years ago

Looks like the try run didn't include gaia unit tests.

Based on visual inspection of the logs, these are probably the only relevant messages (it's bothersome that the log is speckled with "XXX FIXME" unhandled failure codes unrelated to this change):

19:03:44     INFO -  Settings lock not open!
19:03:44     INFO -  System JS : ERROR (null):0 - uncaught exception: 2147500036

Fabrice: any idea?

	Ben Kelly [:bkelly, not reviewing]
	Comment 13 • 11 years ago

I don't fully understand whats happening yet, but I don't think this bug is the cause any more.  I produced the error locally and then tried backing this out.  The error still reproduced.

	Phil Ringnalda (:philor)
	Comment 14 • 11 years ago

That's quite surprising, since it started on b2g-inbound when this was pushed there, on every other trunk tree when this was merged to them, and on aurora when this landed there.

	Ben Kelly [:bkelly, not reviewing]
	Comment 15 • 11 years ago

Maybe my revert needed a clobber?  Seems unlikely, but I'm trying that now.

	Ben Kelly [:bkelly, not reviewing]
	Comment 16 • 11 years ago

Ok, I was running the test wrong.  I needed DESKTOP=0 DEBUG=1, not just DEBUG=1, when I built the gaia profile.

With that change I can now confirm that backing this out fixes the problem.

Similar log messages also:

XXX FIXME : Got a mozContentEvent: accessibility-screenreader
Settings lock not open!
System JS : ERROR (null):0 - uncaught exception: 2147500036
XXX FIXME : Got a mozContentEvent: system-message-listener-ready
--runapp found app: Test Agent, disabling lock screen...
JavaScript error: http://system.gaiamobile.org:8080/js/attention_screen.js, line 329: app is undefined
JavaScript error: http://system.gaiamobile.org:8080/js/window_manager.js, line 1351: runningApps[displayedApp] is undefined
###################################### forms.js loaded
############################### browserElementPanning.js loaded
######################## BrowserElementChildPreload.js loaded
XXX FIXME : Got a mozContentEvent: inputmethod-update-layouts
FTU manifest cannot be found skipping.
###################################### forms.js loaded
############################### browserElementPanning.js loaded
######################## BrowserElementChildPreload.js loaded
--runapp launching app: Test Agent
###################################### forms.js loaded
############################### browserElementPanning.js loaded
######################## BrowserElementChildPreload.js loaded
XXX FIXME : Got a mozContentEvent: inputmethod-update-layouts

(plugin-container:25089): Gtk-WARNING **: Unable to locate theme engine in module_path: "pixmap",

(plugin-container:25089): Gtk-WARNING **: Unable to locate theme engine in module_path: "pixmap",

(plugin-container:25089): Gtk-WARNING **: Unable to locate theme engine in module_path: "pixmap",

(plugin-container:25089): Gtk-WARNING **: Unable to locate theme engine in module_path: "pixmap",
System JS : ERROR resource://gre/modules/FileUtils.jsm:63 - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIProperties.get]
*** UTM:SVC TimerManager:notify - notified @mozilla.org/b2g/webapps-update-timer;1

	Ben Kelly [:bkelly, not reviewing]
	Comment 17 • 11 years ago

To run the tests you first need to ./mach build and ./mach package b2g-desktop.  My .mozconfig looks like:

  ac_add_options --with-ccache

  #
  # B2G Desktop settings
  #
  mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-b2g-desktop
  ac_add_options --enable-application=b2g
  ENABLE_MARIONETTE=1
  ac_add_options --disable-elf-hack
  export CXXFLAGS=-DMOZ_ENABLE_JS_DUMP

  GAIADIR=$topsrcdir/gaia

Then in gaia:

  DESKTOP=0 DEBUG=1 NOFTU=1 make

Then in gaia:

  python -u tests/python/gaia-unit-tests/gaia_unit_test/main.py --binary /srv/mozilla-central/obj-b2g-desktop/dist/bin/b2g-bin  --profile /srv/gaia-master/profile-debug

Fixing paths of course.

You may have to install some python deps.  For example, I had to do "pip install tornado".

Hope that helps!

	[:fabrice] Fabrice Desré Assignee
	Comment 18 • 11 years ago

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #12)
> Looks like the try run didn't include gaia unit tests.
> 
> Based on visual inspection of the logs, these are probably the only relevant
> messages (it's bothersome that the log is speckled with "XXX FIXME"
> unhandled failure codes unrelated to this change):

These are not real failures, just things we plan to improve.

> 19:03:44     INFO -  Settings lock not open!
> 19:03:44     INFO -  System JS : ERROR (null):0 - uncaught exception:
> 2147500036
> 
> Fabrice: any idea?

Not yet, I'm setting up my test runner to reproduce.

	Sid Stamm [:geekboy or :sstamm]
	Comment 19 • 11 years ago

Looks like a failure in mAppStatusCache.Get().  

Based on a scan of the tree, I think the uncaught exception is an NS_ERROR_ABORT thrown probably here:
http://mxr.mozilla.org/mozilla-central/source/dom/settings/SettingsManager.js#171

	Sid Stamm [:geekboy or :sstamm]
	Comment 20 • 11 years ago

Er, no, not a failure in mAppStatusCache.Get(), I was way off.

It's possibly a problem with aRequestPrincipal->GetAppStatus(&status).  My guess is that there's a SettingsLock that's being held when we try to get the app status and so that bit fails ... but that's pure speculation since I don't know this part of the code at all.  I haven't figured out how to hook my debugger up yet.

	[:fabrice] Fabrice Desré Assignee
	Comment 21 • 11 years ago

So, we are rejected 3 loads:
Rejecting loading http://test-agent.gaiamobile.org:8080/common/test/test_url_resolver.js from http://wappush.gaiamobile.org:8080//test/unit/_proxy.html?time1382136609280 (2)
Rejecting loading http://test-agent.gaiamobile.org:8080/common/vendor/test-agent/test-agent.js from http://wappush.gaiamobile.org:8080//test/unit/_proxy.html?time1382136609280 (2)
Rejecting loading http://test-agent.gaiamobile.org:8080/common/test/agent_proxy.js from http://wappush.gaiamobile.org:8080//test/unit/_proxy.html?time1382136609280 (2)

and that leads to the test failures. 2 is the contentType for TYPE_SCRIPT as expected.

	Ben Kelly [:bkelly, not reviewing]
	Comment 22 • 11 years ago

I took the liberty of writing the test bustage up as bug 929172.

	Deian Stefan
	Comment 23 • 11 years ago

Comment on attachment 817937 [details] [diff] [review]
patch

Review of attachment 817937 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsCSPService.cpp
@@ +112,5 @@
> +  uint16_t status;
> +  nsAutoCString contentOrigin;
> +  aContentLocation->GetPrePath(contentOrigin);
> +  if (!mAppStatusCache.Get(contentOrigin, &status)) {
> +    aRequestPrincipal->GetAppStatus(&status);

Should we be using aRequestPrincipal? The rest of the code is using the node principal.

	:gerard-majax [PTO 01/08/2023-10/09/2023]
	Comment 24 • 11 years ago

It's funny, I suspect I have another manifestation of this behavior. While testing an application in B2G Simulator v1.3, there is issues with cookies. My app is making use of XHR mozSystem and withCredentials. It runs well in the v1.2 of the simulator :)