514578 - reftest support for arbitrary manifest URLs

Status: NEW

(Core :: Layout, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: read manifests from any URL

reftests have a dependency on file: URIs currently; it can be handy to pull the manifest from http or jar.  The attached patch makes this work, at the expense of the HTTP tests because those really want to serve an explicit directory.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 398531 [details] [diff] [review]
read manifests from any URL

Whoops, forgot to request review on this.  Shouldn't change behaviour for current tests.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 398531 [details] [diff] [review]
read manifests from any URL

My memory is that I originally designed so that it would be easy to do
this (and originally avoided the assumption that the tests would always
be file: URLs), but that some patches since have made some security
assumption such that this isn't safe to do.

For a start, I think we should refuse (at manifest-read-time) to run
TYPE_SCRIPT reftests that aren't at file: URLs.  (I think just throwing
an exception during manifest reading does the right thing.)

This also still doesn't support HTTP reftests on non-file URLs, so those
should also give an error at manifest-read time.

See also the discussion in bug 371075 about the security implications of
this.

You should also remove this comment:
>  // XXX necessary?  manifestURL guaranteed to be file, others always HTTP


>-    gServer = CC["@mozilla.org/server/jshttp;1"].
>-                  createInstance(CI.nsIHttpServer);
>+
>+    try {
>+        gServer = CC["@mozilla.org/server/jshttp;1"].
>+            createInstance(CI.nsIHttpServer);
>+    } catch (ex) {
>+        gServer = null;
>+    }

Is the idea here to make the server component optional for cases where
there are no HTTP tests?  If so, we should give an error early (at
manifest-read-time) if there are any tests that use HTTP.

>+        dump(ex);
>         dump("REFTEST TEST-UNEXPECTED-FAIL | | EXCEPTION: " + ex + "\n");

No need for this.

+    // is it a file?
+    try {
+        var listURL = aURL.QueryInterface(CI.nsIFileURL);
+        var fis = CC[NS_LOCALFILEINPUTSTREAM_CONTRACTID].
+            createInstance(CI.nsIFileInputStream);
+        fis.init(listURL.file, -1, -1, false);
+        lis = fis.QueryInterface(CI.nsILineInputStream);
+    } catch (ex) {
+    }
+
+    if (lis)
+        return lis;


Instead of try-catch that ignores exceptions from the last three lines,
how about putting this whole thing (including the return) inside:
  if (aURL instanceof CI.nsIFileURL) {
  }
(Plus, XPConnect has some magic so that you don't need the
QueryInterface after you've tested instanceof, and I think it even works
for objects without classinfo, so I suspect you can remove the QI,
though.)

+function GetLineInputStream(aURL)
+{

Maybe worth commenting that both halves of this function (the
nsILineInputStream half and the nsIScriptableInputStream half) assume
that the input is ASCII (and in both cases they cross the C++ to JS
boundary using narrow string types).

+        if (runHttp && (!fileURL || !gServer))
+            continue;

As I mentioned above, just silently absorbing this error isn't ok.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #2)
> (From update of attachment 398531 [details] [diff] [review])
> My memory is that I originally designed so that it would be easy to do
> this (and originally avoided the assumption that the tests would always
> be file: URLs), but that some patches since have made some security
> assumption such that this isn't safe to do.
> 
> For a start, I think we should refuse (at manifest-read-time) to run
> TYPE_SCRIPT reftests that aren't at file: URLs.  (I think just throwing
> an exception during manifest reading does the right thing.)

Hmm, ok.  How important are security issues in our test harness?  Seems odd to be writing tests that have security implications, unless we're explicitly checking that the security infrastructure works.

> This also still doesn't support HTTP reftests on non-file URLs, so those
> should also give an error at manifest-read time.

Well.. I don't really want to give an error, I want to just skip those tests -- the idea is that you should be able to run as many tests as you can using the stock manifests and not have to manually create new manifest files.  The patch as written just skips those, but it should probably be louder about the fact that they were skipped instead.