Status

()

Core
Security: PSM
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: emk, Assigned: emk, NeedInfo)

Tracking

({dev-doc-complete, site-compat})

unspecified
mozilla53
dev-doc-complete, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [psm-assigned], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Chrome will stop the support on May 15th.
(Assignee)

Comment 1

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b2bc765abed
Some spdy/h2 tests fail if I disable npn...
(Assignee)

Comment 2

2 years ago
Node v4.x does not support ALPN :(

Comment 3

2 years ago
Chrome has now stopped supporting NPN in latest Chrome stable. Only ALPN is supported. Firefox should do the same.
Assignee: nobody → VYV03354
Whiteboard: [psm-assigned]
(Assignee)

Comment 6

2 years ago
Created attachment 8762294 [details] [diff] [review]
Enable NPN in tests that use SPDY or HTTP/2

We can't use ALPN until our test infrastructure has node >=5.0.0.
(Assignee)

Updated

2 years ago
Depends on: 1280178
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d762ad7a091bb5e336ee190caaf5c948359dc34
test_spdy.js still fails even with node 5. node-spdy doesn't support SPDY+ALPN?
(Assignee)

Updated

2 years ago
Depends on: 1283871
(Assignee)

Comment 11

2 years ago
Created attachment 8770572 [details]
Bug 1248198 - Remove the "security.ssl.enable_npn" pref.

Review commit: https://reviewboard.mozilla.org/r/64016/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64016/
Attachment #8770572 - Flags: review?(mh+mozilla)
Attachment #8770573 - Flags: review?(dkeeler)
https://reviewboard.mozilla.org/r/64018/#review61060

I'm actually going to redirect to :mcmanus on this one (for one thing, I'm not sure how this impacts spdy and if we want to disable that at the same time?)
Attachment #8770573 - Flags: review?(mcmanus)
Comment on attachment 8770573 [details]
Bug 1248198 - Disable NPN by default

The only practical effect this will have right now is the breaking of websites. It can be done later.
Attachment #8770573 - Flags: review?(mcmanus) → review-
(Assignee)

Comment 17

2 years ago
(In reply to Patrick McManus [:mcmanus] from comment #16)

I'll follow necko owner's decision, but

> The only practical effect this will have right now is the breaking of
> websites. It can be done later.

No websites will break. Affected websites will just use http/1.1 instead of spdy or h2.
(In reply to Masatoshi Kimura [:emk] from comment #17)

> 
> No websites will break. Affected websites will just use http/1.1 instead of
> spdy or h2.

it's true - its a particular definition of break :)
Comment on attachment 8770572 [details]
Bug 1248198 - Remove the "security.ssl.enable_npn" pref.

https://reviewboard.mozilla.org/r/64016/#review61444

Please add more details as to why (especially why that version) in the commit message and/or a comment in the code.
Attachment #8770572 - Flags: review?(mh+mozilla) → review+
(Assignee)

Comment 20

2 years ago
Comment on attachment 8770572 [details]
Bug 1248198 - Remove the "security.ssl.enable_npn" pref.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64016/diff/1-2/
Attachment #8770573 - Attachment description: Bug 1248198 - Disable NPN by default. → Bug 1248198 - Disable NPN by default
Attachment #8770573 - Flags: review- → review?(mcmanus)
(Assignee)

Comment 21

2 years ago
Comment on attachment 8770573 [details]
Bug 1248198 - Disable NPN by default

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64018/diff/1-2/
(Assignee)

Updated

2 years ago
Attachment #8770573 - Flags: review?(mcmanus)
(Assignee)

Comment 22

2 years ago
I didn't mean to request review again (for now). I posted the updated patch for posterity. MozReview is too damn smart...
fwiw a bigger issue here than spdy may be that h2 is widely used with npn for now - largely because it took openssl a long time to ship alpn. Its definitely not in our interests to push that traffic back onto h1.

on beta 48 iirc its about 5:1 in favor of alpn vs npn, but its 50:1 in favor of h2 vs spdy.. (it might be time to get rid of spdy. I'll think about that harder.)
A heads up. We disabled NPN in NSS 3.27 (which I just landed on inbound and should ride trains with FF 51) to see if anything breaks. Only the ability to enable it is removed and we plan on removing the actual code in 3.28 when things don't break too badly.
Flags: needinfo?(mcmanus)
emk: can we land the hasNode patch?  We had to back out NSS 3.27 because the version of node on taskcluster is too old.
Flags: needinfo?(VYV03354)
(Assignee)

Comment 26

2 years ago
(In reply to Martin Thomson [:mt:] from comment #25)
> emk: can we land the hasNode patch?  We had to back out NSS 3.27 because the
> version of node on taskcluster is too old.

I think we can. The patch is already r+'ed and does not affect NPN functionality.
Flags: needinfo?(VYV03354)
(Assignee)

Comment 27

2 years ago
Oh, I recalled why I didn't land it. MozReview does not allow partial landing :( I will have to extract and attach the patch.
(Assignee)

Comment 28

2 years ago
Created attachment 8792151 [details] [diff] [review]
"hasNode" patch for checkin
Attachment #8770572 - Attachment is obsolete: true
Attachment #8792151 - Flags: review+

Comment 30

2 years ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6876f222943
Set "hasNode" only if the node version is >=5.0.0 because older node does not support ALPN. r=glandium
(Assignee)

Updated

2 years ago
Keywords: leave-open
So the "hasNode" patch appears to have totally broke our h2 testing on infra, so right now we have no automated test coverage for that. Please back it out until infra gets upgraded to an appropriate node version.
Oh, just saw comment 24 - so apparently we have no NPN on trunk, and we have no ALPN available for testing, so it's literally impossible to test h2? We need to either revert these changes or pressure releng to upgrade all our nodes to 5.0 or higher, now.

Also, there should be a way to say "this test should count as a failure if it's skipped", as that would've caught this situation instead of us having no h2 testing for the last 2+ weeks...
OK, from talking to people in #releng, it looks like backout of the NPN removal and the hasNode patch is the right way to go - this counts as test bustage (even though it's silent). Not an ideal situation, to be sure (I also applaud the effort to kill NPN!), but better to keep supporting old/experimental tech and have tests than to not be testing our h2 implementation (plus the things that depend on it now, like webpush).
(see comments 32-34)
Flags: needinfo?(VYV03354)
(Assignee)

Comment 36

2 years ago
I added node 5 support on taskcluster in bug  1280178. Is it effectively reverted by bug 1293737? If so, bug 1293737 should be reverted.
Flags: needinfo?(gps)
Flags: needinfo?(dustin)
Flags: needinfo?(VYV03354)
OK, that's a slightly better situation (and indeed, taskcluster builds seem to be running our h2 tests). However, taskcluster xpcshell tests are tier-2. All our tier-1 h2 tests (both linux and os x) are still being skipped because non-tc has not been upgraded.
Ugh, hit "save" too soon. Meant to add:

So, effectively, we still have tier-1 test bustage (even though it's silent) which should be backed out.
(Assignee)

Comment 39

2 years ago
OK, but I can't simply backout this because it will cause (non-silent) bustage. Please ask :mt or :ekr about NSS backout.
Flags: needinfo?(gps)
Flags: needinfo?(dustin)
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #38)
> Ugh, hit "save" too soon. Meant to add:
> 
> So, effectively, we still have tier-1 test bustage (even though it's silent)
> which should be backed out.

Backing out the hasNode patch will only make things fail. I don't think that's what you want. Backing out NSS would leave Aurora with a beta version of NSS, so I don't think that's an option either. Pressuring folks into upgrading infrastructure sounds like the right approach to me.
xpcshell tests for linux64/debug and /asan are tier-1.  Which are you referring to at tier-2?
(In reply to Dustin J. Mitchell [:dustin] from comment #41)
> xpcshell tests for linux64/debug and /asan are tier-1.  Which are you
> referring to at tier-2?

taskcluster are listed as tier-2 (unless I'm reading treeherder wrong). But yes, non-taskcluster xpcshell tests on linux (and os x!) are tier-1, and those are what is silently busted right now.
(Assignee)

Comment 43

2 years ago
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #42)
> (In reply to Dustin J. Mitchell [:dustin] from comment #41)
> > xpcshell tests for linux64/debug and /asan are tier-1.  Which are you
> > referring to at tier-2?
> 
> taskcluster are listed as tier-2 (unless I'm reading treeherder wrong).

Linux x64 opt and pgo TC are listed as tier-2, but debug and asan TC are not (unless I'm reading treeherder wrong).
I also found bug 1242556 was fixed (that is, at least Linux x64 debug TC is tier-1).

> But yes, non-taskcluster xpcshell tests on linux (and os x!) are tier-1, and
> those are what is silently busted right now.

It's normal that a test is disabled on *some* tier-1 platform.
(In reply to Masatoshi Kimura [:emk] from comment #43)
> > But yes, non-taskcluster xpcshell tests on linux (and os x!) are tier-1, and
> > those are what is silently busted right now.
> 
> It's normal that a test is disabled on *some* tier-1 platform.

Yes, but linux and mac are the only opt tier-1 builds that test(ed) h2. And as much as it's good that we have ASAN and debug builds testing h2, we should also have the bits we actually ship testing h2, as well.
(Assignee)

Updated

2 years ago
Depends on: 1309110
Depends on: 1311768
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8770573 - Attachment is obsolete: true
(Assignee)

Comment 46

2 years ago
* This pref is dead sue to upstream change (bug 1288044). So there is no point in keeping the pref.
* Now we have a good test platform coverage.

Comment 47

2 years ago
mozreview-review
Comment on attachment 8770572 [details]
Bug 1248198 - Remove the "security.ssl.enable_npn" pref.

https://reviewboard.mozilla.org/r/64016/#review93986

Yeah, the pref's a no-op. Doesn't make much sense to keep this code around.

::: security/manager/ssl/nsNSSComponent.cpp:1887
(Diff revision 3)
>  
>    SSL_OptionSetDefault(SSL_ENABLE_FALSE_START,
>                         Preferences::GetBool("security.ssl.enable_false_start",
>                                              FALSE_START_ENABLED_DEFAULT));
>  
> -  // SSL_ENABLE_NPN and SSL_ENABLE_ALPN also require calling
> +  // SSL_ENABLE_ALPN also requires calling

nit: it would be great to reformat this documentation text to be as close to 80 characters on each line as possible

(in vim, you can set the text width to 80, select these lines, and use `gq` to do this automatically)
Attachment #8770572 - Flags: review?(dkeeler) → review+
Keywords: leave-open
Priority: -- → P1

Comment 48

2 years ago
mozreview-review
Comment on attachment 8770572 [details]
Bug 1248198 - Remove the "security.ssl.enable_npn" pref.

https://reviewboard.mozilla.org/r/64016/#review94056
Attachment #8770572 - Flags: review?(mcmanus) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 50

2 years ago
:glandium, autoland wants to add you as a reviewer even though you didn't review this part of the patch.
Is this a bug? Is it possible to remove a reviewer?
Flags: needinfo?(mh+mozilla)
I don't know.
Flags: needinfo?(mh+mozilla)

Comment 52

2 years ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e8714a6425c
Remove the "security.ssl.enable_npn" pref. r=keeler,mcmanus
(Assignee)

Comment 53

2 years ago
(In reply to Mike Hommey [:glandium] from comment #51)
> I don't know.

OK, thanks. I landed the patch manually to edit the commit message as expected.

Comment 54

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6e8714a6425c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2017/npn-support-has-been-removed/
Keywords: dev-doc-needed, site-compat
I've added a note to the Fx53 release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/53#HTTPNetworking
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.