939755 - With httpd.js we sometimes don't get the most recent version of the file

Status: NEW

(Testing :: General, defect)

Description

[Julien Wajsberg [:julienw]]

+++ This bug was initially created as a clone of Bug #939729 +++

This bug is to apply the changes from bug 939729 to moz-central's httpd.js.

Comment 1

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Hey Julien, let me know if I can help on this issue :-)

Comment 2

[Julien Wajsberg [:julienw]]

Hey Yuren,

please do, I haven't tried to do it on upstream's httpd.js yet. And maybe we need to introduce a "filter" concept instead of hacking inside the main file.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

apparently this is to ensure that the patch from bug 939729 is applied to all versions of httpd.js.

Comment 4

[Leni Kadali [:leni1] [GMT+3]]

Attachment: With httpd.js we sometimes don't get the most recent version of the file

Applied the changes from bug 939729 to addon-sdk/source/test/addons/e10s-content/lib/httpd.js.

changed addon-sdk/source/test/addons/e10s-content/lib/httpd.js

Comment 5

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8882547 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file

Started with the file addon-sdk/source/test/addons/e10s-content/lib/httpd.js.
The files addon-sdk/source/test/addons/content-permissions/httpd.js and addon-sdk/source/test/addons/content-script-messages-latency/httpd.js had the following message at the top: 

/*
*  NOTE: do not edit this file, this is copied from:
*  https://github.com/mozilla/addon-sdk/blob/master/test/lib/httpd.js
*/

In such a case how would I make the changes to these files, since PRs aren't accepted on GitHub?

Comment 6

[Leni Kadali [:leni1] [GMT+3]]

May I be assigned to the bug please? :)

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8882547 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file

Review of attachment 8882547 [details] [diff] [review]:
-----------------------------------------------------------------

thanks for this patch!  Submitting patches like this is just fine vs github.  The only reason for a r- is the extra whitespace.

::: addon-sdk/source/test/addons/e10s-content/lib/httpd.js
@@ +2654,5 @@
>      }
>      else
>      {
> +        var lastModified;
> +        

nit: there is extra whitespace here on the blank line, it should just be a newline, not a line with spaces/tabs.

@@ +2660,5 @@
>        {
> +          lastModified = toDateString(file.lastModifiedTime);
> +          response.setHeader("Last-Modified", lastModified, false);
> +      }
> +      catch (e) { 

extra space after {

@@ +2661,5 @@
> +          lastModified = toDateString(file.lastModifiedTime);
> +          response.setHeader("Last-Modified", lastModified, false);
> +      }
> +      catch (e) { 
> +          /* lastModifiedTime threw, ignore */ 

extra space after */

@@ +2679,5 @@
> +        dumpn(">>> file was not modified, returning a 304 code");
> +        response.setStatusLine("1.1", 304);
> +        return;
> +      }
> +       

the last 5 blank lines have extra spaces/tabs.

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Leni, thanks for your work on this- I will be on holiday, :ahal can help with reviews and and next steps on this bug or other bugs- he is a great mentor and we work together often!

Comment 9

[Julien Wajsberg [:julienw]]

Hey, given I'm the original author of the patch in Bug 939729 and given the patch isn't very different to what I did, is it possible to have both Leni and my name on this patch ? If that's not possible with hg at least please quote me in the commit log :) Thanks

Comment 10

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Julien Wajsberg [:julienw] from comment #9)
> Hey, given I'm the original author of the patch in Bug 939729 and given the
> patch isn't very different to what I did, is it possible to have both Leni
> and my name on this patch ? If that's not possible with hg at least please
> quote me in the commit log :) Thanks

Hmmm... Sure. Don't see why not. How would I quote you in the commit log? My idea would be to reference you as the author of the patch in Bug 939729 that I'm applying to upstream. Beyond that, I'm not sure. :)

Comment 11

[Leni Kadali [:leni1] [GMT+3]]

Attachment: With httpd.js we sometimes don't get the most recent version of the file ahal

Applied the changes from bug 939729 by Julien Wajsberg [:julienw]
to addon-sdk/source/test/addons/e10s-content/lib/httpd.js.
Removed whitespaces.

Comment 12

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8882786 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file  ahal

I hope this reference in the commit log suits Julien :)
Requesting review from Andrew since Joel's on holiday.

Comment 13

[Andrew Halberstadt [:ahal]]

Hey Leni, looks like this latest patch still has the trailing whitespaces that Joel mentioned previously. Most editors have some way of automatically removing them (or making them visible), you could try googling how to set that up in your editor. You could also just remove them manually, but then you might miss them again in the future :).

Comment 14

[Julien Wajsberg [:julienw]]

Thanks, this works for me :)

Comment 15

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Andrew Halberstadt [:ahal] from comment #13)
> Hey Leni, looks like this latest patch still has the trailing whitespaces
> that Joel mentioned previously. Most editors have some way of automatically
> removing them (or making them visible), you could try googling how to set
> that up in your editor. You could also just remove them manually, but then
> you might miss them again in the future :).

Hmmm... Interesting. I ran ./mach eslint against the file and it didn't show any of them. 
Will do and see.

Comment 16

[Andrew Halberstadt [:ahal]]

Ah, the eslint linter isn't configured to run on every file, it's likely httpd.js is still excluded. You can see the trailing whitespace in the splinter review as the red bars. Thanks!

Comment 17

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Andrew Halberstadt [:ahal] from comment #16)
> Ah, the eslint linter isn't configured to run on every file, it's likely
> httpd.js is still excluded. You can see the trailing whitespace in the
> splinter review as the red bars. Thanks!

Where would I find this splinter review? Apologies in advance if I'm asking something that should 
be obvious.

Comment 18

[Andrew Halberstadt [:ahal]]

No worries! If you looks at your attachment on this bug, you can click the "splinter review" link to see an enhanced diff. Then click on the file name and you'll see the whitespace highlighted in red.

Comment 19

[Leni Kadali [:leni1] [GMT+3]]

Attachment: With httpd.js we sometimes don't get the most recent version of the file ahal

Applied the changes from bug 939729 by Julien Wajsberg [:julienw]
to addon-sdk/source/test/addons/e10s-content/lib/httpd.js.
Removed whitespaces.

Comment 20

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8885852 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file  ahal

The whitespaces were highlighted in Nano in very much the same way as in the splinter review. I hope I've gotten rid of them.

Comment 21

[Leni Kadali [:leni1] [GMT+3]]

The other files I'm planning to make this modification to are as follows:

mozilla-central/addon-sdk/source/lib/sdk/test/httpd.js
mozilla-central/addon-sdk/source/test/addons/content-permissions/httpd.js
mozilla-central/addon-sdk/source/test/addons/content-script-messages-latency/httpd.js
mozilla-central/addon-sdk/source/test/addons/e10s-content/lib/httpd.js
mozilla-central/addon-sdk/source/test/addons/places/lib/httpd.js
mozilla-central/addon-sdk/source/test/lib/httpd.js
mozilla-central/addon-sdk/source/test/test-httpd.js
mozilla-central/netwerk/test/httpserver/httpd.js
mozilla-central/services/sync/tps/extensions/mozmill/resource/stdlib/httpd.js

If there are any I shouldn't be modifying, please let me know :)

Comment 22

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Leni Kadali from comment #21)
> The other files I'm planning to make this modification to are as follows:
> 
> mozilla-central/addon-sdk/source/lib/sdk/test/httpd.js
> mozilla-central/addon-sdk/source/test/addons/content-permissions/httpd.js
> mozilla-central/addon-sdk/source/test/addons/content-script-messages-latency/
> httpd.js
> mozilla-central/addon-sdk/source/test/addons/e10s-content/lib/httpd.js
> mozilla-central/addon-sdk/source/test/addons/places/lib/httpd.js
> mozilla-central/addon-sdk/source/test/lib/httpd.js
> mozilla-central/addon-sdk/source/test/test-httpd.js
> mozilla-central/netwerk/test/httpserver/httpd.js
> mozilla-central/services/sync/tps/extensions/mozmill/resource/stdlib/httpd.js
> 
> If there are any I shouldn't be modifying, please let me know :)

These are the files I don't think I'll be able to modify since their copies no longer exist on GitHub as far as I can tell:

addon-sdk/source/lib/sdk/test/httpd.js:
throw new Error(`This file was removed. A copy can be obtained from:
  https://github.com/mozilla/addon-sdk/blob/master/test/lib/httpd.js`);

addon-sdk/source/test/addons/content-permissions/httpd.js
addon-sdk/source/test/addons/content-script-messages-latency/httpd.js
addon-sdk/source/test/addons/places/lib/httpd.js
/*
*  NOTE: do not edit this file, this is copied from:
*  https://github.com/mozilla/addon-sdk/blob/master/test/lib/httpd.js
*/

I don't think addon-sdk/source/test/test-httpd.js is to be modified judging by its contents.

Comment 23

[Andrew Halberstadt [:ahal]]

Wow, I knew we had more than one copy of httpd.js in the tree, but didn't know there were that many!

The main copy of httpd.js is netwerk/test/httpserver/httpd.js, so that one for sure needs to be modified. I'm not sure if all the others are supposed to be updated or not. Joel, do you know? I think you are right about addon-sdk/source/lib/sdk/test/httpd.js, let's not touch that one.

Comment 24

[Andrew Halberstadt [:ahal]]

Comment on attachment 8885852 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file  ahal

Review of attachment 8885852 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, looks good! Could you also modify the other instances of httpd.js in the same patch please? The most important one is the one under /netwerk

::: addon-sdk/source/test/addons/e10s-content/lib/httpd.js
@@ +2653,5 @@
>        }
>      }
>      else
>      {
> +        var lastModified;

nit: please make this the same indent level as below.

@@ +2661,5 @@
> +          response.setHeader("Last-Modified", lastModified, false);
> +      }
> +      catch (e) {
> +          /* lastModifiedTime threw, ignore */
> +          lastModified = null;

httpd.js uses two space indents, please convert this patch to two spaces as well.

Comment 25

[Andrew Halberstadt [:ahal]]

(In reply to Andrew Halberstadt [:ahal] from comment #23)
> The main copy of httpd.js is netwerk/test/httpserver/httpd.js, so that one
> for sure needs to be modified. I'm not sure if all the others are supposed
> to be updated or not. Joel, do you know? I think you are right about
> addon-sdk/source/lib/sdk/test/httpd.js, let's not touch that one.

Comment 26

[Joel Maher ( :jmaher ) (UTC -8)]

that list of files looks accurate, but why do we have so many copies.  I would edit them all, then file a followup bug to use the build system to copy a master copy of httpd.js to all of the places it is needed during the build phase.

Comment 27

[Leni Kadali [:leni1] [GMT+3]]

Attachment: With httpd.js we sometimes don't get the most recent version of the file ahal

Applied the changes from bug 939729 by Julien Wajsberg [:julienw]
to addon-sdk/source/test/addons/e10s-content/lib/httpd.js.
Removed whitespaces.

Comment 28

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8886265 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file  ahal

Hopefully this isn't too much to review all at once. :-)

Comment 29

[Andrew Halberstadt [:ahal]]

Comment on attachment 8886265 [details] [diff] [review]
With httpd.js we sometimes don't get the most recent version of the file  ahal

Review of attachment 8886265 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks leni! This looks good. I'm going to be PTO tomorrow, but if you need help landing this maybe jmaher can help. Otherwise I'll be back next week and can help you then.

Comment 30

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Andrew Halberstadt [:ahal] from comment #29)
> Comment on attachment 8886265 [details] [diff] [review]
> With httpd.js we sometimes don't get the most recent version of the file 
> ahal
> 
> Review of attachment 8886265 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks leni! This looks good. I'm going to be PTO tomorrow, but if you need
> help landing this maybe jmaher can help. Otherwise I'll be back next week
> and can help you then.

My apologies, but I have never landed a patch myself. If you don't mind, could you guide me through how to do it?

Comment 31

[Andrew Halberstadt [:ahal]]

Sorry Leni, I forgot to do this. You won't be able to land without special commit access to the repository. The first step is to test this patch out on try server. If the results look good then I can land it for you, and if not I'll let you know. I'll leave the needinfo here to remind myself to do this.

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e96c2ecd3a47ba60c83b7b27c1f98685a6243f5a

Comment 32

[Andrew Halberstadt [:ahal]]

Uh oh, looks like there are a lot of test failures in the try push.

Leni, this isn't your fault or anything, your patch is correct. This is happening because we have *a lot* of tests using httpd.js, and even a slight change will often cause a bunch of them to fail. I don't think hunting down and fixing all the failing tests should fall on you, and I suspect this wasn't actually a good first bug after all :(

I think the best thing for now is to leave this bug alone, you've done your part, and hopefully we can figure out how to land this without breaking any tests at some point in the near future. I'm really sorry about that. But if you want to find something else to work on in the meantime, I'd be happy to help you find something.

Joel, I'm removing the good first bug tag, how do you want to proceed here?

Comment 33

[Joel Maher ( :jmaher ) (UTC -8)]

I think this bug should be tackled when 57 is off of nightly (or even when it is in release).  We are changing a lot of test scheduling and frameworks now, possibly some of these will be easier to copy via the build system, or at the very least we will be running fewer tests and the failures will be easier to understand.

Comment 34

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Andrew Halberstadt [:ahal] from comment #32)
> Uh oh, looks like there are a lot of test failures in the try push.
> 
> Leni, this isn't your fault or anything, your patch is correct. This is
> happening because we have *a lot* of tests using httpd.js, and even a slight
> change will often cause a bunch of them to fail. I don't think hunting down
> and fixing all the failing tests should fall on you, and I suspect this
> wasn't actually a good first bug after all :(
> 
> I think the best thing for now is to leave this bug alone, you've done your
> part, and hopefully we can figure out how to land this without breaking any
> tests at some point in the near future. I'm really sorry about that. But if
> you want to find something else to work on in the meantime, I'd be happy to
> help you find something.
No worries. I think I have all the JS good first bugs bookmarked so finding what to do next shouldn't be a problem :-)

Comment 35

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.