570596 - httpd server shouldn't include guid identifiers in its URI

Status: VERIFIED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With the current state of the httpd server it will include random guid identifiers into the url. That makes it hard for us to implement some tests with local test data, especially for tests for the search engines. It would require us to create those opensearch xml files on the fly. It would be better when we could use static urls.

Current behavior:
http://localhost:43336/d9746a82d6e8-9c45-9cfe-ad49af65db7d/testfile.js

Wanted behavior:
http://localhost:43336/testfile.js

I wonder if there is any reason why we are using the guids by default. Jeff, can you explain it please?

Comment 1

[Jeff Hammel]

(In reply to comment #1)
> I wonder if there is any reason why we are using the guids by default. Jeff,
> can you explain it please?

I assume you mean Jeff Walden?  I have no clue.

The code that does this is here: http://github.com/mikeal/mozmill/blob/master/mozmill/extension/resource/modules/frame.js#L311

{{{
  if (ns == undefined) {
    ns = uuidgen.generateUUID().toString().replace('-', '').replace('{', '').replace('}','');
  }
  var lp = Components.classes["@mozilla.org/file/local;1"]
             .createInstance(Components.interfaces.nsILocalFile);
  lp.initWithPath(os.abspath(directory, this.current_file));
  this.httpd.registerDirectory('/'+ns+'/', lp);
}}}

If there is no reason not to take this out, then this should fix the problem.
}}}

Comment 2

[Jeff Hammel]

Attachment: patch to remove UUIDs from the server

this will/should remove the UUIDs from the http server. untested

Comment 3

[Jeff Hammel]

(In reply to comment #2)
> Created an attachment (id=450152) [details]
> patch to remove UUIDs from the server
> 
> this will/should remove the UUIDs from the http server. untested

Doing this causes 5 new failures (the one that looks fixed is probably a red herring):

> diff before.txt after.txt 
2d1
< Test Failed : testRestoreHomeToDefault in /home/jhammel/mozmill/src/mozmill-tests/firefox/testPreferences/testRestoreHomepageToDefault.js
11a11
> Test Failed : testScrollBackgroundTabIntoView in /home/jhammel/mozmill/src/mozmill-tests/firefox/testTabbedBrowsing/testBackgroundTabScrolling.js
14a15,16
> Test Failed : testOpenInBackgroundTab in /home/jhammel/mozmill/src/mozmill-tests/firefox/testTabbedBrowsing/testOpenInBackground.js
> Test Failed : testPermissionsDisabled in /home/jhammel/mozmill/src/mozmill-tests/firefox/testPrivateBrowsing/testDisabledPermissions.js
34a37,38
> Test Failed : testPopUpBlocked in /home/jhammel/mozmill/src/mozmill-tests/firefox/testPopups/testPopupsBlocked.js
> Test Failed : testUndoTabFromContextMenu in /home/jhammel/mozmill/src/mozmill-tests/firefox/testSessionStore/testUndoTabFromContextMenu.js
42c46
< Passed 186 :: Failed 29 :: Skipped 7
---
> Passed 182 :: Failed 33 :: Skipped 7

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 450152 [details] [diff] [review]
patch to remove UUIDs from the server

>   if (ns == undefined) {
>-    ns = uuidgen.generateUUID().toString().replace('-', '').replace('{', '').replace('}','');
>+      ns = '/';
>   }
>+  else {
>+      ns = '/' + ns + '/';
>+  }

We have an indentation of 2 characters in JS. Can you use the comprehensive style (?:) here?

>   var lp = Components.classes["@mozilla.org/file/local;1"]
>              .createInstance(Components.interfaces.nsILocalFile);

Put the dot at the end of the first line and align createInstace with Components. Haven't we Cc and Ci defined as short names?

>+  this.httpd.registerDirectory(ns, lp);
>+  return 'http://localhost:'+this.http_port+ns

Please add spaces between the operands.

> }
> Collector.prototype.initTestModule = function (filename) {
>   var test_module = loadFile(filename, this);

Can you please add a blank line between those two functions while we are already here?

One thing I'm just wondering about, why do we have two slashes in the url between the port and the path?

With those changes r=me.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #3)
> Doing this causes 5 new failures (the one that looks fixed is probably a red
> herring):

You should better run those tests with Namoroka or Shiretoko builds. Tests for 1.9.3 are currently broken in some areas. That said I can't see that it should break something on our end.

Comment 6

[Jeff Hammel]

Attachment: updated patch to remove UUIDs

carrying r+ from whimboo and obsolescent patch

Comment 7

[Jeff Hammel]

(In reply to comment #4)
> (From update of attachment 450152 [details] [diff] [review])
> >   if (ns == undefined) {
> >-    ns = uuidgen.generateUUID().toString().replace('-', '').replace('{', '').replace('}','');
> >+      ns = '/';
> >   }
> >+  else {
> >+      ns = '/' + ns + '/';
> >+  }
> 
> We have an indentation of 2 characters in JS. Can you use the comprehensive
> style (?:) here?

Corrected.  Sorry about that.
 
> >   var lp = Components.classes["@mozilla.org/file/local;1"]
> >              .createInstance(Components.interfaces.nsILocalFile);
> 
> Put the dot at the end of the first line and align createInstace with
> Components. Haven't we Cc and Ci defined as short names?

Not at file-scope, no.  Fixed, even though it was not part of my original patch.

> >+  this.httpd.registerDirectory(ns, lp);
> >+  return 'http://localhost:'+this.http_port+ns
> 
> Please add spaces between the operands.

Done.

> > }
> > Collector.prototype.initTestModule = function (filename) {
> >   var test_module = loadFile(filename, this);
> 
> Can you please add a blank line between those two functions while we are
> already here?

Done.

> One thing I'm just wondering about, why do we have two slashes in the url
> between the port and the path?

I think I figured this out.  At least, I have a theory.  Originally (and in the old patch), the check was for ns being undefined.  I have replaced this with `if(!ns)` which should cover the case where ns is '' or other non-value.  I hope this fixes.

> With those changes r=me.

New patch at https://bugzilla.mozilla.org/attachment.cgi?id=450263

Comment 8

[Jeff Hammel]

(In reply to comment #5)
> (In reply to comment #3)
> > Doing this causes 5 new failures (the one that looks fixed is probably a red
> > herring):
> 
> You should better run those tests with Namoroka or Shiretoko builds. Tests for
> 1.9.3 are currently broken in some areas. That said I can't see that it should
> break something on our end.

Who is going to be the point-person(s) on fixing tests?

Comment 9

[Jeff Hammel]

 
> > One thing I'm just wondering about, why do we have two slashes in the url
> > between the port and the path?
> 
> I think I figured this out.  

It turns out I was wrong.  I have no idea where the extra slash comes from.  Are the tests putting it in?  That's the only place I would know to look.  I'm unfamiliar with httpd, so it's possible it's happening internally there.  

I would guess fixing this will at fix the 5(ish) failing tests caused by this patch.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 450263 [details] [diff] [review]
updated patch to remove UUIDs

>+  else {
>+    ns = '/' + ns + '/';
>+  }

Nearly fine now. else should be one line higher. I will update it before I check this in.

(In reply to comment #8)
> Who is going to be the point-person(s) on fixing tests?

I'm the one who assigns tasks to Geo, Anthony, and myself. It's still quite a lot of work to do.

Regarding the double slashes, it's the problem in our tests. I have figured out why it happens and Anthony will work on fixing it with his work on bug 568008.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed as:
http://github.com/mikeal/mozmill/commit/a43c8163a8f171eb07f01bfeb89a5a6ceed170ba

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Tested with the developer version of Mozmill and the patch checked in. Works fantastic with and without a namespace => Verified.