Honza Bambas (:mayhemer) Assignee
	Description • 16 years ago

This bug is needed to satisfy some reftest where chrome privileges are needed.

The attachment is first draft that turns reftest framework to:
- run in its own temporary profile
- access all test files through a chrome URL
- allow the framework to work with chrome URLs
- fixes test for bug 398289 to work again
It is tested on windows and mac os x. 

There is one problem with the test on mac: it fails but just because of gained focus on the ok button (a shadow on it) during the reference rendering. Is this known problem? It seems all other test are passing.

I mark this as security problem because there is straight link between the two bugs that would obviosly reveal bug 295994 issue.

	Jeff Walden [:Waldo]
	Comment 1 • 16 years ago

I for one would veryveryveryverymuch prefer if the script used to do this were written in Python, which I find to be both more readable and more writable (and is what Mozilla, in general, is using for build scripts going forward).  build/pgo/automation.py.in and build/pgo/profileserver.py.in should have everything you need to create a clean profile and run applications.

	Honza Bambas (:mayhemer) Assignee
	Comment 2 • 16 years ago

(In reply to comment #1)
> I for one would veryveryveryverymuch prefer if the script used to do this were
> written in Python

I expect that :) Ok, no problem to do it. Just a note, I am not expect to neither perl nor python ;)

	Honza Bambas (:mayhemer) Assignee
	Comment 3 • 16 years ago

This is the python version using automation.py.in. Perfectly works on both the windows and mac machines.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 4 • 16 years ago

David, any chance of a review here? This patch is blocking a security fix we'd like to take in a dot-release.

	David Baron :dbaron:
	Comment 5 • 16 years ago

(In reply to comment #0)
> The attachment is first draft that turns reftest framework to:
> - run in its own temporary profile

It seems good to have a script to do this, although I'd prefer it not be required.

> - access all test files through a chrome URL

Why?  I'd really prefer not to do this.

> - allow the framework to work with chrome URLs

Which part of the changes are needed to do this?  (What didn't work before?)

	Honza Bambas (:mayhemer) Assignee
	Comment 6 • 16 years ago

David, thanks for look at the patch. But, please, ignore the first comment. It is related to obsolete patch. take a look at attachment 317723 [details] [diff] [review] that introduces python script for test run.

The only problem with reftest framework was checking of privileges of the test page: privileges of the reftest.list file URL was tested, not the privileges of the test URL itself. When changed, we can use chrome:// urls as well as normal ones for testing.

Reason to introduce this change: with changes from bug 295994 test for bug 398289 fails because it lacks chrome privileges.

	Daniel Veditz [:dveditz]
	Comment 7 • 16 years ago

Is there any reason this bug needs to remain a hidden security bug? It looks to me like it's a neutral test-infrastructure bug.

	Honza Bambas (:mayhemer) Assignee
	Comment 8 • 16 years ago

(In reply to comment #7)
> Is there any reason this bug needs to remain a hidden security bug? It looks to
> me like it's a neutral test-infrastructure bug.
> 

It is explained in the description. If we open this bug it is clear that it was allowed to store persistent attributes w/o the xul privileges. Clever heads could discover this easily.

	David Baron :dbaron:
	Comment 9 • 16 years ago

Could you respond to the points and questions in comment 5?

	Honza Bambas (:mayhemer) Assignee
	Comment 10 • 16 years ago

I already did in comment 6. Comment 5 touches obsolete comment and attachment. The note "- access all test files through a chrome URL" is probably wrong. There is no such change in both of the patches. 

I actually don't understand the objection "It seems good to have a script to do this, although I'd prefer it not be required." The second patch attached is using python script based on automation.py as suggested in comment 1. What exactly did you mean?

	David Baron :dbaron:
	Comment 11 • 16 years ago

OK, let's start over.

What does your patch do?

	Honza Bambas (:mayhemer) Assignee
	Comment 12 • 16 years ago

Ok, it does three thinks:

1. allows adding chrome:// URLs to reftest lists and lets such test then run with chrome privileges.
2. introduces a script that allows reftests be run in a separate profile (as mochitest is).
3. changes test for bug 398289 to work again as expected.

The first change is achieved just by change here: https://bugzilla.mozilla.org/attachment.cgi?id=317723&action=diff#layout/tools/reftest/reftest.js_sec1. I changed the way of privileges test.

	David Baron :dbaron:
	Comment 13 • 16 years ago

What makes the content of a reftest file appear at chrome://tests/content/... ?

I don't think the resulting checkLoadURI call really makes any sense.  I think you should just remove the two calls to checkLoadURI instead.  (The idea was really about moving towards allowing a reftest.list on the Web rather than at a local file, but the code prevents that in other ways.)

(If we keep it, I don't think the newURI call inside it really makes sense; document.location is known to be absolute.  I also prefer document.location.href over document.location.toString().)

I think sayrer should probably also review the Makefile.in and runreftest.py.in changes.

	Honza Bambas (:mayhemer) Assignee
	Comment 14 • 16 years ago

(In reply to comment #13)
> What makes the content of a reftest file appear at chrome://tests/content/... ?
> 

This is trick copied from the mochitest: create chrome directory in the profile directory and add content to it (this manages the runtests.py script). Gecko then accesses this as a normal chrome.

> I don't think the resulting checkLoadURI call really makes any sense.  I think
> you should just remove the two calls to checkLoadURI instead.  (The idea was
> really about moving towards allowing a reftest.list on the Web rather than at a
> local file, but the code prevents that in other ways.)
> 

I will do it.

> (If we keep it, I don't think the newURI call inside it really makes sense;
> document.location is known to be absolute.  I also prefer
> document.location.href over document.location.toString().)
> 
> I think sayrer should probably also review the Makefile.in and runreftest.py.in
> changes.

I will add him to review request. Thanks.
> 

	David Baron :dbaron:
	Comment 15 • 16 years ago

(In reply to comment #14)
> (In reply to comment #13)
> > What makes the content of a reftest file appear at chrome://tests/content/... ?
> > 
> 
> This is trick copied from the mochitest: create chrome directory in the profile
> directory and add content to it (this manages the runtests.py script). Gecko
> then accesses this as a normal chrome.

Where is the code that does this?  Is it in runreftest.py?  If so, see comment 5 (which you said was not relevant).

	Honza Bambas (:mayhemer) Assignee
	Comment 16 • 16 years ago

Now I understand. The code is in in the runreftest.py script. See the line 'manifestPath = BROWSER_DIR + "/chrome/tests.manifest"' and bellow. And one correction: the chrome dir is not created at the profile directory but at the bin dir. This is really how mochitest does it.

Your objection to do not allow all test files be accessible by chrome:// url is in place. Then we must explicitly list files that have to be there and list them in the tests.manifest file. This puts additional effort on test writers.

	David Baron :dbaron:
	Comment 17 • 16 years ago

So the test that you're adding will only work when runreftest.py is used, and it will fail when run directly from the command line as we now do?

	Honza Bambas (:mayhemer) Assignee
	Comment 18 • 16 years ago

Exactly. I remember I was instructed by someone (probably Jeff but not sure of it) to write a script running the tests and actually depreciate command line support. Chrome support is difficult or even impossible while using command line to run the tests.

	David Baron :dbaron:
	Comment 19 • 16 years ago

So, it really would have been nice if you had said that sometime before comment 18.  I asked explicitly in comment 5 and you said in comment 10 that comment 5 was not relevant, and in comment 12 you explicitly said that the script *allows* (not *requires*).

If we're going to do that, it means that:
 * this would need to be landed in stages, with changes to the build automation in the middle
 * it makes our reftests less likely to be usable by other browser vendors


Is there anything else important that you omitted from the list in comment 12?  It's a lot easier to review a patch if you know what it's intended to do.  (This is now pretty much at the bottom of my priority list for code reviews.)

	David Baron :dbaron:
	Comment 20 • 16 years ago

To be clear, I'm quite happy to have an optional script for creating a clean profile and running reftests in it; I'm a bit less happy about making the script mandatory.

	Honza Bambas (:mayhemer) Assignee
	Comment 21 • 16 years ago

Sorry for the confusion, I am not expert to the testing code. Maybe we could have 3 ways to run tests:

1. By the command line as it is now
2. By the script, without chrome tests (the same functionality as the command line)
3. By the script, just the chrome tests.

I push on this bug because patch for important security bug could not be landed because a very different bug's test fails.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 22 • 16 years ago

I absolutely think that we need to run our reftests from a custom profile, as well as with chrome access. 

I'm not sure how much value there is in sharing the actual test runner with other browsers. Seems more important to be able to share the actual tests and the manifest files with them.

	David Baron :dbaron:
	Comment 23 • 16 years ago

If the actual tests require chrome privileges and have to be at chrome: URLs, we can't share them with other browsers.

What about separating the reftests that require chrome privileges into a different directory and manifest?

	David Baron :dbaron:
	Comment 24 • 16 years ago

And I'd note that not all reftests are in layout/reftests/, and I'd somewhat like more of them not to be.  It looks like this patch only does chrome privilege stuff for the tests that are in it.

	David Baron :dbaron:
	Comment 25 • 16 years ago

Did you consider rewriting this one reftest as a mochitest and moving on?

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 26 • 16 years ago

I really think that we'll want chrome reftests for more than just this one. I've already had to do workarounds because we don't run reftests in a special profile.

However having it enabled in only some specific manifests sounds like a good idea.

	(not currently active) Ted Mielczarek
	Comment 27 • 16 years ago

Only enabling chrome in certain manifests would probably also make it easier to maintain the chrome -> file path mapping, since you wouldn't have to support every possible dir that reftests are in...

	Honza Bambas (:mayhemer) Assignee
	Comment 28 • 16 years ago

Based on discussions and feedback it seems there is no need for the framework extension. Rather turn ref-tests to chrome "ref-test" mochitests. Example: attachment 340540 [details] [diff] [review].