Fast-path for certified apps CSP

RESOLVED FIXED in Firefox 26

Status

()

defect
--
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

({perf})

unspecified
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:koi+, firefox25 wontfix, firefox26 fixed, firefox27 fixed, b2g-v1.2 fixed)

Details

(Whiteboard: [c=progress p= s= u=1.2][qa-])

Attachments

(1 attachment)

Since we don't let 3rd parties change this CSP we can hardcode the check until the full c++ rewrite is done.
Posted patch patchSplinter Review
Assignee: nobody → fabrice
Attachment #817937 - Flags: review?(sstamm)
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.
Attachment #817937 - Flags: review?(sstamm) → review+
Status: NEW → ASSIGNED
blocking-b2g: --- → koi+
Whiteboard: [c=progress p= s= u=1.2]
+ koi+ per conversations with Vivien & Fabrice
+ Updated dependency relationship with bug 925004 per conversation with Fabrice.
Blocks: 925004
No longer depends on: 925004
https://hg.mozilla.org/mozilla-central/rev/ac2eb4f684ad
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
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
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)
Flags: needinfo?(bkelly)
(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.
Flags: needinfo?(bkelly)
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?
Flags: needinfo?(fabrice)
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.
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.
Maybe my revert needed a clobber?  Seems unlikely, but I'm trying that now.
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
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!
(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.
Flags: needinfo?(fabrice)
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
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.
I took the liberty of writing the test bustage up as bug 929172.
Whiteboard: [c=progress p= s= u=1.2] → [c=progress p= s= u=1.2][qa-]
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.
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 :)
You need to log in before you can comment on or make changes to this bug.