Closed Bug 1248198 Opened 8 years ago Closed 8 years ago

Drop NPN support

Categories

(Core :: Security: PSM, defect, P1)

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: emk, Assigned: emk, NeedInfo)

References

(
URL
)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [psm-assigned])

Attachments

(4 files, 1 obsolete file)

patch
1.18 KB, patch
Details | Diff | Splinter Review
Enable NPN in tests that use SPDY or HTTP/2
4.59 KB, patch
Details | Diff | Splinter Review
Bug 1248198 - Remove the "security.ssl.enable_npn" pref.
58 bytes, text/x-review-board-request
keeler
: review+
mcmanus
: review+
Details
"hasNode" patch for checkin
2.53 KB, patch
emk
: review+
Details | Diff | Splinter Review 
Chrome will stop the support on May 15th.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b2bc765abed
Some spdy/h2 tests fail if I disable npn...
Node v4.x does not support ALPN :(
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]
Attached patch patch — — Splinter Review 

    
    

    
  


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

    
  
Depends on: 1280178

    
    

    
  
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?

    
  
Depends on: 1283871

    
    

    
  
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?)

    
    

    
  
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-

    
    

    
  
(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+

    
    

    
  
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)

    
    

    
  
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/

    
  

        Attachment #8770573 -
        Flags: review?(mcmanus)

    
    

    
  
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.)
See Also:  → 1288044

    
    

    
  
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)

    
    

    
  
(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)

    
    

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

    
    

    
  


  

        Attachment #8770572 -
        Attachment is obsolete: true

        Attachment #8792151 -
        Flags: review+

    
    

    
  
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

    
  
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)

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
(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.

    
  
Depends on: 1309110

    
    

    
      
  

    
  

        Attachment #8770573 -
        Attachment is obsolete: true

    
    

    
  
* 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 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+

    
    

    
  
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+

    
    

    
      
  

    
    

    
  
: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)

    
    

    
  
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

    
    

    
  
(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.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/6e8714a6425c
Status: NEW → RESOLVED
Closed: 8 years ago

          status-firefox53:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: