Open Bug 988497 Opened 6 years ago Updated 2 years ago

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

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: mihneadb, Assigned: mihneadb)

Details

Attachments

(1 file, 1 obsolete file)

As per the discussion in bug 936181, httpd.js should not allow using hardcoded ports by default. The behavior can be changed, explicitly.
Attachment #8397300 - Flags: review?(ahalberstadt)
Assignee: nobody → mihneadb
Status: NEW → ASSIGNED
Try run: https://tbpl.mozilla.org/?tree=Try&rev=c042afda8000

:ahal, can you help with running this on Cedar? Thanks!
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.
Attachment #8397300 - Flags: review?(ahalberstadt) → review+
Attachment #8397300 - Attachment is obsolete: true
Comment on attachment 8397330 [details] [diff] [review]
Don't allow by default hardcoded ports in httpd.js

Whoops! Keeping r+.
Attachment #8397330 - Flags: review+
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.
Attachment #8397330 - Flags: review+ → feedback+
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.
Attachment #8397330 - Flags: review?(jwalden+bmo)
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 :)
Attachment #8397330 - Flags: review?(jwalden+bmo)
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?
There were a few left to begin with. ("run-sequentially")
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.
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.
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 :)
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.
Component: httpd.js → General
You need to log in before you can comment on or make changes to this bug.