Closed Bug 929172 Opened 11 years ago Closed 11 years ago

gaia unit tests broken in b2g desktop due to fast CSP enhancement

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: bkelly, Assigned: julienw)

References

Details

Attachments

(1 file)

github PR
(deleted), text/x-github-pull-request
yurenju
: review+
Details 
After bug 927493 landed the python gaia unit tests began getting stuck at startup.  Output log looks like this:

  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
So here's what happening:
The fast CSP is rejecting 3 resource 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

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

Rejecting loading http://test-agent.gaiamobile.org:8080/common/test/agent_proxy.js from http://wappush.gaiamobile.org:8080//test/unit/_proxy.html?time1382136609280

These look like legitimate rejections since these are cross domain script loads. In this case, we let the code also run the JS CSP checks to get error reporting. But in these cases, there is actually no CSP associated for these resources, so we just keep rejecting them.

The original code sets the aDecision to default to ACCEPT, and then didn't run any check for these loads... That sounds worse than with the fast path in my view, but if needed I can push a followup to reset the decision to ACCEPT in the fast path falltrough.
So there's a document not initialized properly?  Some document with APP_STATUS_CERTIFIED that wasn't originally an app?  (That's the only way a document with certified app would fail the fast path but not the original CSP checks.)
Blocks: 928073
(In reply to Fabrice Desré [:fabrice] from comment #1)
> The original code sets the aDecision to default to ACCEPT, and then didn't
> run any check for these loads... That sounds worse than with the fast path
> in my view, but if needed I can push a followup to reset the decision to
> ACCEPT in the fast path falltrough.

It seems to me the normal and fast paths should probably use the same algorithm.  If we think that algorithm is wrong, then it should be fixed in both places in a different bug.  Part of fixing the algorithm could be adapting the tests to work with the new approach.  Just my opinion here.
(In reply to Ben Kelly [:bkelly] from comment #3)
> (In reply to Fabrice Desré [:fabrice] from comment #1)
> > The original code sets the aDecision to default to ACCEPT, and then didn't
> > run any check for these loads... That sounds worse than with the fast path
> > in my view, but if needed I can push a followup to reset the decision to
> > ACCEPT in the fast path falltrough.
> 
> It seems to me the normal and fast paths should probably use the same
> algorithm.  If we think that algorithm is wrong, then it should be fixed in
> both places in a different bug.  Part of fixing the algorithm could be
> adapting the tests to work with the new approach.  Just my opinion here.

I don't disagree here. Doing a temp patch to get the fast path similar to the old one is easy, but I think we'll still need to figure out why these tests are not failing...
Blocks: 923880
(In reply to Fabrice Desré [:fabrice] from comment #1)
> So here's what happening:
> The fast CSP is rejecting 3 resource 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
> 
> 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
> 
> Rejecting loading
> http://test-agent.gaiamobile.org:8080/common/test/agent_proxy.js from
> http://wappush.gaiamobile.org:8080//test/unit/_proxy.html?time1382136609280

Hmm.  Looking at this a bit closer, the profile used for the tests has the httpd extension.  This extension seems to pass Access-Control-Allow-Origin headers for test-agent.gaiamobile.org:

  https://github.com/mozilla-b2g/gaia/blob/master/tools/extensions/httpd/bootstrap.js#L170
  https://github.com/mozilla-b2g/gaia/blob/master/tools/extensions/httpd/bootstrap.js#L212

How does the fast CSP path interact with the Access-Control-Allow-Origin headers?
Flags: needinfo?(fabrice)
Another option might be able to send a custom Content-Security-Policy header for gaia desktop HTTPD requests. If this overrides/bypasses the fastpath, it seems like it may be the easiest change. (I'm not sure how that works with the fastpath though)
The fast path doesn't do anything with headers (we don't have any with the app:// protocol). I really think that it's a test bug, but we can add code to the fast path to make it work exactly like the old code if needed.
Flags: needinfo?(fabrice)
(In reply to Fabrice Desré [:fabrice] from comment #7)
> The fast path doesn't do anything with headers (we don't have any with the
> app:// protocol). I really think that it's a test bug, but we can add code
> to the fast path to make it work exactly like the old code if needed.

Should this be true even with DEBUG=1 profile?  Aren't we using http:// protocol scheme?  The URLs in comment 1 reference http://, not app://.

To be honest, I don't know enough about this code to really understand how its supposed to work or what the correct fix to the tests would be.
James, do you think it would be possibly to drop the cross-domain loads for test-agent in the gaia unit tests?
Flags: needinfo?(jlal)
We can drop this but we also need to hack httpd so we can load the same content in the domain of the test (and document this somehow). This is not very complicated:  https://github.com/mozilla-b2g/gaia/blob/master/tools/extensions/httpd/bootstrap.js.
Flags: needinfo?(jlal)
Ah! A faster/better way to deal with this is just move the test-agent code to shared/. shared/ did not exist when I wrote this thing and the current situation sucks anyway.
Why is this failing on TBPL but apparently passing on Travis?
Yuren / Julien: Do either if you have a day to make this work? If not I will pick this up the following week.
Flags: needinfo?(yurenju.mozilla)
Flags: needinfo?(felash)
(In reply to Jonathan Griffin (:jgriffin) from comment #12)
> Why is this failing on TBPL but apparently passing on Travis?

Tbpl Gaia unit tests run with the python runner and b2gdesktop.  Travis uses the JavaScript runner and the Firefox browser.
Sadly not this week for me, sorry...
Flags: needinfo?(felash)
me too, pretty busy this week.
Flags: needinfo?(yurenju.mozilla)
(In reply to Ben Kelly [:bkelly] from comment #14)
> (In reply to Jonathan Griffin (:jgriffin) from comment #12)
> > Why is this failing on TBPL but apparently passing on Travis?
> 
> Tbpl Gaia unit tests run with the python runner and b2gdesktop.  Travis uses
> the JavaScript runner and the Firefox browser.

Actually Python unit tests should run the same on both environments.
Summary: python unit tests broken due to fast CSP enhancement → gaia unit tests broken in b2g desktop due to fast CSP enhancement
(In reply to Julien Wajsberg [:julienw] from comment #17)
> (In reply to Ben Kelly [:bkelly] from comment #14)
> > (In reply to Jonathan Griffin (:jgriffin) from comment #12)
> > > Why is this failing on TBPL but apparently passing on Travis?
> > 
> > Tbpl Gaia unit tests run with the python runner and b2gdesktop.  Travis uses
> > the JavaScript runner and the Firefox browser.
> 
> Actually Python unit tests should run the same on both environments.

ok, my bad, we're really talking about unit tests and not UI tests, and I mixed up.

So, you're perfectly right Ben.
Assignee: fabrice → felash
Attached file github PR (deleted) — 

    
    

    
  
So, with this patch, I now run everything from the app's hostname. This still works in Firefox.

Could someone try to run unit tests with b2g-desktop with this patch? You need to build a new profile since httpd.js has been changed.

    
    

    
  
I tried to add a travis job to run the tests inside b2g but I don't know if I configured it correctly so we'll see.

    
    

    
  
The tests run in B2G ! https://travis-ci.org/mozilla-b2g/gaia/jobs/15731399

But there are a lot of failures so I'm gonna move the job to the expected failure list.

    
    

    
  
Comment on attachment 8350223 [details]
github PR

* changes httpd.js to look for /common URLs in the test-agent directory
* changes the test-agent boilerplate to always use the test's hostname
* add a NO_LOCK_SCREEN env variable for disabling the lockscreen at build time
* add a "unit tests in b2g" travis job
---
 .travis.yml                                        |  4 +++-
 Makefile                                           |  3 +++
 build/settings.js                                  |  5 ++---
 .../test-agent/common/test/boilerplate/_proxy.html |  2 +-
 tests/travis_ci/test_agent/before_script           | 23 ----------------------
 tests/travis_ci/test_agent/install                 | 12 -----------
 tests/travis_ci/test_agent/script                  |  3 ---
 tests/travis_ci/unit-tests-in-b2g/before_script    | 22 +++++++++++++++++++++
 tests/travis_ci/unit-tests-in-b2g/install          |  9 +++++++++
 tests/travis_ci/unit-tests-in-b2g/script           |  3 +++
 .../travis_ci/unit-tests-in-firefox/before_script  | 23 ++++++++++++++++++++++
 tests/travis_ci/unit-tests-in-firefox/install      | 12 +++++++++++
 tests/travis_ci/unit-tests-in-firefox/script       |  3 +++
 tools/extensions/httpd/content/httpd.js            |  6 +++++-
 14 files changed, 86 insertions(+), 44 deletions(-)
 delete mode 100755 tests/travis_ci/test_agent/before_script
 delete mode 100755 tests/travis_ci/test_agent/install
 delete mode 100755 tests/travis_ci/test_agent/script
 create mode 100755 tests/travis_ci/unit-tests-in-b2g/before_script
 create mode 100755 tests/travis_ci/unit-tests-in-b2g/install
 create mode 100755 tests/travis_ci/unit-tests-in-b2g/script
 create mode 100755 tests/travis_ci/unit-tests-in-firefox/before_script
 create mode 100755 tests/travis_ci/unit-tests-in-firefox/install
 create mode 100755 tests/travis_ci/unit-tests-in-firefox/script


I noticed that I can't run B2G-Desktop on a profile that was already used, it just doesn't work. We'll need to eventually fix this but at least this will work good enough for the tests in Travis and TBPL.

        Attachment #8350223 -
        Flags: review?(yurenju.mozilla)

    
  
Blocks: tbpl-gaia-unit

    
  
Blocks: 952302

    
    

    
  
overall looks good.

but for httpd.js, could we use |nsIHttpServer.registerPathHandler| to handle /common path? it would help us to migrate httpds.js in gaia source tree to upstream.

you can reference this commit (it isn't landed yet):
https://github.com/yurenju/gaia/commit/8bb05767bad41390bfe5408a5ee94c5e4567a2e8#diff-1

and we should extract all modification in httpd.js to bootstrap.js and update our httpd.js to latest one (in another bug :-)

    
    

    
  
(In reply to Yuren Ju [:yurenju] from comment #24)
> overall looks good.
> 
> but for httpd.js, could we use |nsIHttpServer.registerPathHandler| to handle
> /common path? it would help us to migrate httpds.js in gaia source tree to
> upstream.
> 
> you can reference this commit (it isn't landed yet):
> https://github.com/yurenju/gaia/commit/
> 8bb05767bad41390bfe5408a5ee94c5e4567a2e8#diff-1

Ok, I'll try this.

However, be careful, we really need that the changes in bug 939729 applies to these handlers as well. We _need_ that the cache headers are correctly set.

I wonder if we can have something like a pre-filter instead of a handler (this is what I would use in the Java world ;) ). I'll try something.

> 
> and we should extract all modification in httpd.js to bootstrap.js and
> update our httpd.js to latest one (in another bug :-)

We need bug 939755 in before using the upstream.

    
    

    
  
Comment on attachment 8350223 [details]
github PR

thanks Julien! please set review flag again to me if you have updated pull request.

        Attachment #8350223 -
        Flags: review?(yurenju.mozilla)

    
    

    
  
Comment on attachment 8350223 [details]
github PR

Hey Yuren,

I changed the pull request, I think it's a lot cleaner now :) We can also do better in httpd.js.

The tests in B2G Desktop are failing, we'll need to turn on some prefs (especially the Promise object that is normally only available in certified apps). We can move forward with this in another bug (the goal is of course to have the B2G Desktop tests green as well).

        Attachment #8350223 -
        Flags: review?(yurenju.mozilla)

    
    

    
  
Comment on attachment 8350223 [details]
github PR

Oh man, that's super clean than before! but it doesn't work for l10n because we change the path of properties files to a relative path such as ../../shared for properties file in shared directory and this path will be used in httpd.js before your refactoring, so we need to change for it.

please reference this document for build a customized l10n gaia:
https://developer.mozilla.org/en-US/Firefox_OS/Building#Building_multilocale

        Attachment #8350223 -
        Flags: review?(yurenju.mozilla)

    
    

    
  
Ah I knew I could break this and was crossing my fingers ;) I'll fix this and request review again soon!

    
    

    
  
BTW, I'm working on bug 900182 and probably will conflict with your pull request in bootstrap.js.

    
    

    
  
Thanks for letting me know!
Blocks: 784681

    
    

    
  
Just updated the PR, waiting for a Travis run and doing some more tests before requesting review.

    
    

    
  
Comment on attachment 8350223 [details]
github PR

Hey Yuren,

I made a separate commit containing only the follow-ups, so that you can see only what I changed. Of course I'll squash before merging.

The code simplifies how the localization is handled. Especially, we now simply use "registerDirectory" in bootstrap.js, and the code in httpd.js is changing the requested URL, much like a Rewrite engine would work. I think it should be easy to rewrite this using more a Filter approach, but since it was not the goal of this patch I didn't want to change this too much.



Here are the tests I did with curl:

curl -H 'Host: sms.gaiamobile.org:8080' http://localhost:8080/shared/resources/languages.json => I get the configured languages.json
curl -H 'Host: sms.gaiamobile.org:8080' http://localhost:8080/shared/locales/date.ini => I get a modified date.ini
curl -H 'Host: sms.gaiamobile.org:8080' http://localhost:8080/shared/locales/date/date.it.properties => I get the italian file
curl -H 'Host: sms.gaiamobile.org:8080' http://localhost:8080/shared/locales/date/date.fr.properties => I get the file from gaia-l10n repository (I changed it so that I could compare)
curl -H 'Host: sms.gaiamobile.org:8080' http://localhost:8080/locales/sms.it.properties => I get the italian file
curl -H 'Host: sms.gaiamobile.org:8080' http://localhost:8080/locales/sms.fr.properties => I get the french file from gaia-l10n (I changed it as well)
curl -H 'Host: sms.gaiamobile.org:8080' http://localhost:8080/manifest.webapp => I get a translated manifest.webapp file

So far so good, but please test as well!


I also enabled the Promise object in all DEBUG=1 profiles, so that the errors in the B2G Travis jobs are much reduced.

So requesting review :)

        Attachment #8350223 -
        Flags: review?(yurenju.mozilla)

    
    

    
  
Comment on attachment 8350223 [details]
github PR

Julian, \o/ r=yurenju

I try to test it on Windows but looks we couldn't use nightly on windows to run gaia for a period of time.

        Attachment #8350223 -
        Flags: review?(yurenju.mozilla) → review+

    
    

    
  
master: 403637566676df775877013652e10d9a168edbd7

thanks, this is awesome!

I'll file bugs for the failures in B2G.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED

    
    

    
  
now I got a lot of conflict LOL

    
    

    
  
Heh, hopefully easy to resolve :)

    
    

    
  
Fabrice, I suddenly have a doubt here.

AFAIK the test-agent is not privileged nor certified (see [1]). MDN is saying that CSP is used only for privileged and certified apps [2].

So, I have 2 questions:
* Do we also use CSP for simply installed apps ?
* Or are the preinstalled apps privileged by default ?

Thanks !

[1] https://github.com/mozilla-b2g/gaia/blob/master/test_apps/test-agent/manifest.webapp
[2] https://developer.mozilla.org/en-US/Apps/CSP
Flags: needinfo?(fabrice)

    
    

    
  
(In reply to Julien Wajsberg [:julienw] from comment #39)
> Fabrice, I suddenly have a doubt here.
> 
> AFAIK the test-agent is not privileged nor certified (see [1]). MDN is
> saying that CSP is used only for privileged and certified apps [2].
> 
> So, I have 2 questions:
> * Do we also use CSP for simply installed apps ?

Only for privileged apps.

> * Or are the preinstalled apps privileged by default ?

They are what their manifest says. All gaia is certified, the marketplace is privileged.

Note that the test-container app is certified and has the full list of permissions.
Flags: needinfo?(fabrice)

    
    

    
  
AFAIK the test-container is used for mochitests, not for unit tests.

Unit tests are run by the test-agent, and the test-agent does not seem privileged (although I'll check on a real device).

I'll try to do CSP-forbidden stuff on a simply non-privileged installed app today...

    
  
Flags: needinfo?(felash)

    
    

    
  
Filed bug 963137.
Flags: needinfo?(felash)

    
    

    
  
after 3 month, I found l10n feature for DEBUG=1 mode doesn't work anymore after this bug landed.

STR:
1. LOCALES_FILE=../l10n/languages.json LOCALE_BASEDIR=../l10n DEBUG=1 make
2. nightly -profile ./profile-debug
3. curl -H 'Host: sms.gaiamobile.org:8080' http://localhost:8080/shared/locales/date/date.pl.properties

expected:
get date.pl.properties content

actually:
file not found.

I'll fix this issue because I'm working on 968666 and need to support l10n in DEBUG=1 mode, just let you know :-)

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: