988497 - Don't allow by default hardcoded ports in httpd.js

Status: ASSIGNED

(Testing :: General, defect)

Description

[Mihnea Dobrescu-Balaur (:mihneadb)]

As per the discussion in bug 936181, httpd.js should not allow using hardcoded ports by default. The behavior can be changed, explicitly.

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Don't allow by default hardcoded ports in httpd.js

Comment 2

[Mihnea Dobrescu-Balaur (:mihneadb)]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=c042afda8000

:ahal, can you help with running this on Cedar? Thanks!

Comment 3

[Andrew Halberstadt [:ahal]]

Comment on attachment 8397300 [details] [diff] [review]
Don't allow by default hardcoded ports in httpd.js

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

Thanks, looks good to me! Yep, I'll get it landed on Cedar.

::: netwerk/test/httpserver/httpd.js
@@ +518,5 @@
> +    if ((typeof _ALLOW_HARDCODED_PORTS === 'undefined' ||
> +         !_ALLOW_HARDCODED_PORTS) &&
> +         port != -1)
> +    {
> +      dump("Cannot use hardcoded httpd.js ports in XPCShell tests.\n");

nit: theoretically this isn't necessarily XPCShell.

Comment 4

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Don't allow by default hardcoded ports in httpd.js

Comment 5

[Mihnea Dobrescu-Balaur (:mihneadb)]

Comment on attachment 8397330 [details] [diff] [review]
Don't allow by default hardcoded ports in httpd.js

Whoops! Keeping r+.

Comment 6

[Andrew Halberstadt [:ahal]]

Oh, your try run was pushed from an old m-c rev.. I pulled m-c and did a new push:
https://tbpl.mozilla.org/?tree=Try&rev=fe502197be5e

Also realized that I probably shouldn't be reviewing httpd.js changes. I'll try to hunt down someone who can.. probably ted.

Comment 7

[Andrew Halberstadt [:ahal]]

Comment on attachment 8397330 [details] [diff] [review]
Don't allow by default hardcoded ports in httpd.js

According to whimboo, Waldo can review this.. Waldo, feel free to pass the buck if that's not the case.

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 8397330 [details] [diff] [review]
Don't allow by default hardcoded ports in httpd.js

Oops, I think I should have looked at that try run before flagging :)

Comment 9

[Andrew Halberstadt [:ahal]]

I guess we'll need to pass in ALLOW_HARDCODED_TESTS in mochitest. Not sure why desktop xpcshell is failing, did new tests get added that use hardcoded ports?

Comment 10

[Mihnea Dobrescu-Balaur (:mihneadb)]

There were a few left to begin with. ("run-sequentially")

Comment 11

[Andrew Halberstadt [:ahal]]

Oh right. Were you still planning to do the patch you mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=936181#c12 ? That would be a prerequisite for this.

Comment 12

[Mihnea Dobrescu-Balaur (:mihneadb)]

I thought this was the patch for that. :) I just created a separate bug since that is an intermittent failure of a particular test.

We could make a patch that sets the allow_hardcoded to true for the tests that fail, but maybe some of the existing tests with issues can be improved.

Comment 13

[Andrew Halberstadt [:ahal]]

Ah, ok.. I thought you were asking review because you wanted to land it. In that case we can push to Cedar, but as a prereq I'd like to not break every mochitest :)

Comment 14

[Mihnea Dobrescu-Balaur (:mihneadb)]

Absolutely. That's what you suggested in the first place. :)

I guess we could set that var true in the mochitest harness and take a look at the xpcshell tests first? But it would probably have to be people more knowledgeable about the tests (owners), since I remember fixing all those that I could fix.