Extend reftest framework to support chrome URLs

RESOLVED WONTFIX

Status

Testing
Reftest
RESOLVED WONTFIX
10 years ago
10 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse meta])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Created attachment 317525 [details] [diff] [review]
First draft

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.
Attachment #317525 - Flags: review?(dbaron)

Comment 1

10 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.
(Assignee)

Comment 2

10 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 ;)
Status: NEW → ASSIGNED
(Assignee)

Comment 3

10 years ago
Created attachment 317723 [details] [diff] [review]
Python version, 1

This is the python version using automation.py.in. Perfectly works on both the windows and mac machines.
Attachment #317525 - Attachment is obsolete: true
Attachment #317723 - Flags: review?(dbaron)
Attachment #317525 - Flags: review?(dbaron)
David, any chance of a review here? This patch is blocking a security fix we'd like to take in a dot-release.
(Assignee)

Updated

10 years ago
Blocks: 443017
(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?)
(Assignee)

Comment 6

10 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.
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.
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Whiteboard: [sg:nse meta]
(Assignee)

Comment 8

10 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.
Could you respond to the points and questions in comment 5?
(Assignee)

Comment 10

10 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?
OK, let's start over.

What does your patch do?
(Assignee)

Comment 12

10 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.
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.
(Assignee)

Comment 14

10 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.
> 

(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).
(Assignee)

Comment 16

10 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.
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?
(Assignee)

Comment 18

10 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.
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.)
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.
(Assignee)

Comment 21

10 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.
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.
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?
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.
Did you consider rewriting this one reftest as a mochitest and moving on?
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.
Summary: Extend reftest framework to suport chrome URLs → Extend reftest framework to support chrome URLs

Updated

10 years ago
Group: core-security
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...
(Assignee)

Comment 28

10 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].
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WONTFIX
(Assignee)

Updated

10 years ago
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
(Assignee)

Updated

10 years ago
Attachment #317723 - Flags: review?(dbaron)
Component: Testing → Reftest
Product: Core → Testing
Version: Trunk → unspecified
QA Contact: testing → reftest
You need to log in before you can comment on or make changes to this bug.