WIP: WebcryptoAPI support #558

r-l-x · 2016-12-21T16:23:21Z

Warning: This branch is based on the branch from #798

This PR implements a few cryptographic functions using HTML5 Web Crypto API when available. This allows a major performance boost when compared to the basic go standard library implementation.

The HTML5 Web Crypto API is available in most in recent browsers, but please note that with Chrome, the page must be loaded from a secure origin (over HTTPS or from localhost). If there is a problem when calling the Web Crypto API, the code uses the Go standard library implementation as a fallback.

This PR optimizes the following functions:

sha256.Sum256
Hashing a 10MB buffer now takes 45ms with Chrome instead of 1400ms (30x faster)
x509.checkSignature (for ECDSA and RSA signatures)
This optimizes ECDSA and RSA signatures signature checks during X509 certificates verification (typically Certificate.Verify). x509.TestECDSA now runs in 60ms instead of 20s (300x faster). Note that DSA signatures are not supported by the Web Crypto API.

Note that the same approach cannot be used to optimize asymmetric signing or signature check functions calls (e.g. rsa.[Sign|Verify]) as defined in the Go standard library, because in the standard library the digest is computed outside the function, but when using the WebCryptoAPI functions, the digest must be computed by the function itself.

See #554

dmitshur · 2016-12-21T19:50:42Z

build/build.go

@@ -290,7 +290,7 @@ func parseAndAugment(pkg *build.Package, isTest bool, fileSet *token.FileSet) ([
 			switch d := decl.(type) {
 			case *ast.FuncDecl:
 				if replacedDeclNames[funcName(d)] {
-					d.Name = ast.NewIdent("_")
+					d.Name.Name = "_gopherjs_overriden_" + d.Name.String()


What you wrote here can be simplified slightly to d.Name.Name = "_gopherjs_overriden_" + d.Name.Name.

If you're assigning to d.Name.Name, you must know d.Name isn't nil, so calling d.Name.String() instead of just d.Name.Name isn't needed.

The word "overriden" is misspelled, it should be "overridden".

You've addressed point 2 but still not 1. Did you miss it, or are you not making the change because you disagree it's an improvement?

Sorry, that was a mistake from me. I'll update the code.

Point 1 is now fixed as suggested.

dmitshur · 2016-12-21T19:54:10Z

I didn't notice any problem with dead code elimination (on my current project, the generated javascript code has slightly the same size), but I'm not sure if there are other points we can check ?

Nice, that's promising! I will try it out on a larger project (that imports more of standard library) and see how it does.

It makes sense that the difference wouldn't be huge, because GopherJS overrides a modest, not large, amount of standard library identifiers. So even if they're now included, it's likely not to be a big deal. And if nothing references them, it's possible DCE still eliminates them. I don't know how advanced the current DCE algorithm is and how this will affect it, perhaps @neelance would know.

dmitshur · 2016-12-21T19:56:53Z

compiler/natives/src/crypto/sha256/sha256.go

+	"github.com/gopherjs/gopherjs/js"
+)
+
+var ErrWebCryptoFailed = errors.New("crypto: Web Crypto API call failed")


Should this be an exported variable? I don't think we want user code to be able to use it.

This is right with the current code, but in the near future this error code will be useful for each of the modules under crypto.

Can you elaborate on how it will be used?

I'm concerned about this because I don't think we should be adding to the API surface of crypto/... packages in standard library, as that would make user code incompatible with Go.

Can you elaborate on how it will be used?

The idea is to have a specific error code returned when a webcrypto method fails with a NotSupportedError. Then most cryptographic functions implementation (in each crypto/* package) would look like:

func foo() error { err := webcrypto_foo() if err == ErrWebCryptoUnsupported { err = _gopherjs_overriden_foo() } return err }

In order to make this error code visible from every package under crypto, the only way I can think of is to make it public.

that would make user code incompatible with Go.

I didn't think about that. This code is not meant to be used by calling applications, but only in gopherjs, in the various crypto packages.
In the last version of the code, I moved the non standard public stuff in a new package crypto/webcrypto. Is that ok? Should we put this kind of code somewhere else (like under the js package) ?

dmitshur · 2016-12-21T19:59:34Z

compiler/natives/src/crypto/sha256/sha256.go

+		}
+	}
+	return subtle
+}


Would it make more sense to execute this code once, at package init time, and save results?

Instead of calling it once for every crypto.Sum256 call.

Similar to how net/http package does it for figuring out the transport to use.

dmitshur · 2016-12-21T20:02:17Z

compiler/natives/src/crypto/sha256/sha256.go

+	case resSlice:= <-resCh:
+		copy(res[:], resSlice)
+	}
+	return


Style-wise, I highly suggest avoiding naked returns because they're less readable.

Naked returns are okay if the function is a handful of lines. Once it's a medium-sized function, be explicit with your return values. Corollary: it's not worth it to name result parameters just because it enables you to use naked returns.

Source: https://github.com/golang/go/wiki/CodeReviewComments#naked-returns

dmitshur · 2016-12-21T20:03:14Z

Overall, this looks really good! Thanks for working on this @r-l-x.

I've left some comments that came to mind looking at it, but this is a great start.

flimzy · 2016-12-21T21:56:33Z

compiler/natives/src/crypto/sha256/sha256.go

+		// The WebCryptoAPI call failed: fallback by calling the implementation from
+		// the Go standard library
+		return _gopherjs_overriden_Sum256(data)
+	}


Under what circumstances might an error occur here? Clearly, if the subtle API isn't supported, but if the subtle API itself returns an error, we'll also fall back to the Go standard library.

I wonder if there might ever be a situation where subtle is present, but constantly failing, which would lead to worse performance (by virtue of calling JS then Go every time), rather than better. Maybe that will never happen, so my concern might be unwarranted.

But if the concern has any validity, perhaps it would make sense to de-register the subtle API if an error is returned, so that subsequent calls go straight to the Go standard lib?

That's a good point.
Generally speaking for crypto functions, the only cases I can think of where I would expect this to happen are:

When the function or one of the parameters is not supported (but the function is still defined). I'm not sure which exception we will get though, I guess a NotSupportedError.

With Chrome when the origin is insecure (NotSupportedError).

When there is a problem with the arguments, e.g. (depending on the algorithm) decryption with a wrong key or wrong authentication tag (According to the W3C spec, we should get an OperationError in this case).

We could try to check if the error code is NotSupportedError and then remember that the webcrypto implementation of this function shouldn't be called, but NotSupportedError may also mean that while the function itself is supported, a parameter, typically a key length or mode of operation is not supported (see https://diafygi.github.io/webcrypto-examples/), so that would be counterproductive in this case to globally avoid the JS implementation of the function.

The performance impact of calling the JS implementation when we get a NotSupportedError is about 5ms under Chrome on insecure origin: http://www.gopherjs.org/playground/#/jlVJjSdJeh

5ms doesn't sound like enough to worry about to me. At least not at this stage. Micro-optimizations like that should come much later :)

r-l-x · 2016-12-22T11:22:33Z

I guess the next step is to implement a generic way to call a JS function returning a promise, so that we don't have to create a channel, implement the calllbacks and call select() for each crypto function we want to wrap. The idea is to have something similar to what @kothar did here.

@shurcooL do you think we could add a CallAsync function in js/js.go ? This kind of construction would be useful globally, e.g. for the fetch transport.

r-l-x · 2016-12-22T15:15:47Z

Another point that would be really useful is to be able to check the error type when a promise fails. Typically, for a decryption function, depending on the mode (let's say AES-GCM), an OperationError will be thrown if the tag check fails, indicating that the key is incorrect or the message was corrupted. In this case, it's useless to call the native go function. But if the exception thrown is NotSupportedError, then it's totally appropriate to fallback to the go standard library.

So I would typically like to call instanceof in order to check the JS error type, but apparently it's not supported as is in gopherjs, it seems that jsbuiltin is needed for that ?

What would you recommend in order to do this in gopherjs ?

flimzy · 2016-12-22T15:39:14Z

So I would typically like to call instanceof in order to check the JS error type, but apparently it's not supported as is in gopherjs, it seems that jsbuiltin is needed for that ?

If you don't want to depend on jsbuiltin (probably should avoid a dependency on that, for the standard lib), you could copy the functionality, by including your own wrapper around instanceOf in an included .js file, the same way jsbuiltin does it. It's a pretty trivial wrapper.

flimzy · 2016-12-22T15:43:05Z

I guess the next step is to implement a generic way to call a JS function returning a promise, so that we don't have to create a channel, implement the calllbacks and call select() for each crypto function we want to wrap. The idea is to have something similar to what @kothar did here.

This can also be done with sync.WaitGroup, but I think a channel is the right approach to avoid a dependency on the sync package.

No reason you can't abstract that into your own helper function(s) for use in this lib, though.

I did something similar to wrap promises in my go-pouchdb package. Maybe the same approach would work here?

r-l-x · 2016-12-22T16:24:21Z

No reason you can't abstract that into your own helper function(s) for use in this lib, though.

Yes, I agree. I was thinking that it would be convenient to have a method with a *js.Object receiver (like Call()), but I can do something similar in the crypto package for now.

flimzy · 2016-12-22T16:26:42Z

Yes, I agree. I was thinking that it would be convenient to have a method with a *js.Object receiver (like Call()), but I can do something similar in the crypto package for now.

That does indeed sound generally useful.

Something like:

func (o *Object) CallPromise(name string, args ...interface{}) (*Object, error)

?

Might be worth creating a separate issue for this. I would probably use that if it existed. Although it might be better handled in a separate library.

r-l-x · 2016-12-22T16:59:04Z

Another thing that would be really helpful: Is there an easy way to launch the tests of the Go standard library in the browser ?

kothar · 2016-12-22T17:13:03Z

Not sure if it helps, but this was the best way I could find to run tests in the browser. Very useful when I was working on the crypto stuff with different node/browser behaviour.
https://bitbucket.org/mikehouston/browsertests/src

It may be possible to use by importing that package into the stdlib tests, but I've not tried it.

flimzy · 2016-12-22T18:34:58Z

compiler/natives/src/crypto/subtle/subtle.go

+		return nil, ErrCryptoNotFound
+	}
+
+	// First check that the method exists (if it doesn't, Call() would panic, so it's better to check first)


Not saying this is a bad approach, but an alternative would be to catch the panic with a recover in a deferred function. Doing so might also be a bit more robust, as it would catch any other runtime panics in an odd JS implementation or something.

flimzy · 2016-12-22T18:40:10Z

Another thing that would be really helpful: Is there an easy way to launch the tests of the Go standard library in the browser ?

You can compile any test to a .js file, then load it in a browser and watch the JS console for output. It's a bit cumbersome, but it works.

I spent some time about a year ago on trying to make this work better, but the GopherJS runtime is missing some features that I think are essential to automate the process.

dmitshur · 2016-12-23T18:38:09Z

I haven't tried and don't know if it's possible, but can you change the kind of a top-level package identifier?

E.g., the real sha256.Sum256 in https://godoc.org/crypto/sha256#Sum256 is a func:

func Sum256(data []byte) [Size]byte {
	...
}

If we can change the kind while overriding, we could make it a variable, like this:

var Sum256 = func() func(data []byte) [Size]byte {
	if webCryptoAvailable {
		return webCryptoImplementationOfSum256
	}
	return _gopherjs_overridden_Sum256
}()

Have you already considered doing it like that? Thoughts?

r-l-x · 2016-12-27T15:29:15Z

var Sum256 = func() func(data []byte) [Size]byte {
	if webCryptoAvailable {
		return webCryptoImplementationOfSum256
	}
	return _gopherjs_overridden_Sum256
}()

I see your point, but one of the problems with this kind of approach is to know when we should call the webcrypto implementation. It doesn't seem feasible to me to determine this a priori. Let's take a few examples:

If the code is run from an insecure origin in Chrome, the SubleCrypto interface is available, but whenever a function is called, it will fail with a NotSupportedError.
The same thing will happen (SubtleCrypto available, but NotSupportedError thrown) when you call a method that's not supported by the browser (and there are a few, even with recent Chrome/Firefox, see https://diafygi.github.io/webcrypto-examples/).
And it can be even more complicated because a method may be supported by the browser but the specific algorithm or key length you requested may be unsupported (e.g. AES-GCM 192bits key generation with Chrome).

So it makes sense to me to first call the webcrypto implementation and then, if it failed because the requested operation was not supported, call the go implementation.
I guess the ideal behavior would be to remember that this specific method (including algorithm/key length) failed, so that the next time the program calls it we don't have to try the webcrypto implementation, but this seems quite complicated and error prone.

r-l-x · 2016-12-28T15:25:01Z

I'm looking at how to run the standard library tests in the browser. With gopherjs test, it's really easy to run in the command line:

$ gopherjs test -v crypto/sha256
gopherjs: Source maps disabled. Use Node.js 4.x with source-map-support module for nice stack traces.
=== RUN   TestGolden
--- PASS: TestGolden (0.17s)
=== RUN   TestSize
--- PASS: TestSize (0.00s)
=== RUN   TestBlockSize
--- PASS: TestBlockSize (0.00s)
=== RUN   TestBlockGeneric
--- PASS: TestBlockGeneric (0.01s)
PASS
warning: system calls not available, see https://github.com/gopherjs/gopherjs/blob/master/doc/syscalls.md
ok  	crypto/sha256	0.595s

But when I generate the code using gopherjs test -v -c crypto/sha256, and load the generated javascript in a browser, I only see in my console:

syscall.go:43 PASS
syscall.go:25 warning: system calls not available, see https://github.com/gopherjs/gopherjs/blob/master/doc/syscalls.md

@kothar

Not sure if it helps, but this was the best way I could find to run tests in the browser. Very useful when I was working on the crypto stuff with different node/browser behaviour.
https://bitbucket.org/mikehouston/browsertests/src
It may be possible to use by importing that package into the stdlib tests, but I've not tried it.

If I get it right, the trick is to inject the desired arguments in os.Args at init time, is that right ?

kothar · 2016-12-29T11:58:19Z

@r-l-x Yep, looks like you got it working, in case it is helpful, here's the original discussion with @shurcooL about tests: https://gophers.slack.com/archives/gopherjs/p1472378655000172

r-l-x · 2017-01-09T17:51:03Z

Ok the current implementation seems good to me, with great performance improvements. We could of course add more cryptographic fonction implementations later, but it would be actually quite tricky for most of them.
For example, in order to implement RSA OAEP encryption, I think we need to use the reflection to recognize what the provided hash fonction is. This is feasible, but I guess that could come in a second (probably more controversial) PR.

What do you think ?

flimzy · 2017-01-09T19:01:46Z

compiler/natives/src/crypto/sha256/sha256.go

+		// | 2.7kB  | 2.400ms   | 2.022ms*  | 710ns     | 160ns*    |
+		// | 100kB  | 774ms     | 24ms*     | 215ms     | 16ms*     |
+		// | 10MB   | 1.632s    | 41ms*     | 1.76s     | 74ms*     |
+		// +--------+-----------+-----------+-----------+-----------+


Do you have an easy way to run these benchmarks? I'd be curious to see how these results compare on different hardware configurations.

Why is the webcrypto version slower for small buffers? Where does it spend the time?

@flimzy I mostly used the gopherJS playground, I don't have the link to the script right now, but I can add it to the discussion when I'm back on my PC.

@neelance I don't know where the time is spent, but from the measurements I did, there is definitely something specific to Chrome, it seems like a webcrypto promise cannot give an answer in less than 2ms. It would be interesting to see if it's the same for any promise. Firefox doesn't have this problem. I can try to profile it.

@flimzy Here is a rather clean way to launch the benchmarks: http://www.gopherjs.org/playground/#/dPF9AaIxVg. Works as is with Firefox, but with Chrome in order to avoid the insecure origin problem, you have to copy the code, load the playground over HTTPS while accepting the security warning and paste the code.

@neelance From the profiling I did (using this in Chrome: ~~http://www.gopherjs.org/playground/#/dPF9AaIxVg~~ http://www.gopherjs.org/playground/#/7uJwxm1UYd), the main thread is idle for most of the time, waiting for the response from the webcrypto API. I'm not sure how to profile the webcryptoAPI thread.

@neelance I think you are right about the channel operation. This example seems to show that the latency occurs between the channel write (in the promise callback) and the channel read outside the promise: http://www.gopherjs.org/playground/#/btm0GV_mQY (2ms per operation).

On the opposite, this example http://www.gopherjs.org/playground/#/uhe5jIrcpA shows that the function call and the hash computation is pretty fast (70µs in my test).

Hey @r-l-x, could you please try the benchmarks again with the scheduler branch of GopherJS? I think it may resolve the 2ms latencies you were seeing.

The branch got merged into master, please test that one instead. :-)

@neelance is latest master deployed to gopherjs playground?

Running https://gopherjs.github.io/playground/#/xN-qiC0TQk on gopherjs playground right now results in len 2000 being slightly faster for webcrypto, 1750 being about equal, and <=1500 being faster for the go implementation

(running on ubuntu16.04 x64 with chrome)

Also ran the 'stop benchmark after result, before sending result in channel' test and am seeing webcrypto faster until 64 bytes and less

(Merged the results from the above test and the one from the previous comment)
len: 1
go 20000 58100 ns/op 0.02 MB/s
webcrypto 1000 1267000 ns/op 0.00 MB/s
webcrypto No Chan 133800 ns/op

len: 64
go 10000 103200 ns/op 0.62 MB/s
webcrypto 1000 1255000 ns/op 0.05 MB/s
webcrypto No Chan 134000 ns/op

len: 128
go 10000 142300 ns/op 0.90 MB/s
webcrypto 1000 1248000 ns/op 0.10 MB/s
webcrypto No Chan 134800 ns/op

len: 256
go 5000 221800 ns/op 1.15 MB/s
webcrypto 1000 1258000 ns/op 0.20 MB/s
webcrypto No Chan 137200 ns/op

len: 512
go 3000 402000 ns/op 1.27 MB/s
webcrypto 1000 1255000 ns/op 0.41 MB/s

len: 1024
go 2000 705000 ns/op 1.45 MB/s
webcrypto 1000 1256000 ns/op 0.82 MB/s

len: 1200
go 2000 824000 ns/op 1.46 MB/s
webcrypto 1000 1246000 ns/op 0.96 MB/s

len: 1500
go 2000 1012500 ns/op 1.48 MB/s
webcrypto 1000 1254000 ns/op 1.20 MB/s

dmitshur · 2017-01-09T19:05:53Z

This is feasible, but I guess that could come in a second (probably more controversial) PR. What do you think ?

I agree, let's leave out additional enhancements for future PRs. It's better to get this one fully reviewed and merged first.

neelance · 2017-01-09T20:00:09Z

compiler/natives/src/crypto/rand/rand.go

@@ -5,6 +5,7 @@ package rand
 import (
 	"errors"

+	"github.com/gopherjs/gopherjs/compiler/natives/src/crypto/webcrypto"


I think github.com/gopherjs/gopherjs/js/webcrypto might be better, since natives is not meant to be imported directly.

I did the move: 02e0350

neelance · 2017-01-09T20:01:30Z

compiler/natives/src/crypto/rand/rand.go

-	if crypto != js.Undefined {
-		if crypto.Get("getRandomValues") != js.Undefined {
+	if webcrypto.Crypto != nil {
+		if webcrypto.Crypto.Get("getRandomValues") != js.Undefined {


Please merge the if statements.

r-l-x · 2017-03-14T08:30:21Z

I can't see the error message in Circle CI. I just keep seeing a spinning circle, and an error in browser console:

Yes, I'm not sure why, but in order to see the error with CircleCI, I need to fold and unfold the red part.

Here is our error:

/home/ubuntu/gopherjs/test.790433663:39
$packages["reflect_test"]=(function(){var $pkg={},$init,E,F,S,G,A,T,H,B,I,J,C,K,L,M,N,O,D,P,Q,R,AP,AQ,AR,BQ,BS,BT,BU,BY,BZ,CA,CI,CK,CN,CZ,DG,DJ,DK,DV,DZ,EA,EB,EC,ED,EE,EF,EH,EI,EJ,EK,EN,EP,EQ,ER,ES,ET,EU,EV,EW,EX,EY,EZ,FA,FB,FC,FD,FE,FF,FG,FH,FI,FJ,FK,FR,FS,FU,FY,FZ,GB,GU,GV,GW,GZ,HI,HJ,HK,HL,HM,HN,HO,HR,HS,IX,JA,JB,JC,JD,JE,JF,JG,JH,JI,JJ,JK,JL,JM,JN,JO,JP,JQ,JR,JS,JT,JU,JV,JW,JX,JY,KB,KG,KH,KN,KO,KS,LA,LD,LP,LT,LU,MK,MT,MU,MV,VE,VG,XP,XR,XZ,YK,YM,YN,YR,YS,YT,YU,YV,YW,YX,YY,ZE,ZF,ZG,ZH,ZI,ZJ,ZL,ZN,ZO,ZP,ZQ,ZR,ZT,ZU,ZW,ZY,ZZ,AAC,AAI,AAQ,AAX,AAY,ABE,ABF,ABH,ABI,ABL,ABM,ABN,ABP,ABQ,ABR,ABS,ABU,ACE,ACF,ACL,ACV,AFB,AFE,AFF,AFG,AFH,AFI,AFJ,AFK,AFL,AFP,AFT,AFU,AFV,AFW,AGL,AGM,ND,NE,NF,NG,NH,NI,NJ,NK,NL,NM,NN,NO,NP,NQ,NR,NS,NT,NU,NV,NW,NX,NY,NZ,OA,OB,OC,OD,OE,OF,OG,OH,OI,OJ,OK,OL,OM,ON,OO,OP,OQ,OR,OS,OT,OU,OV,OW,OX,OY,OZ,PA,PB,PC,PD,PE,PF,PG,PH,PI,PJ,PK,PL,PM,PN,PO,PP,PQ,PR,PS,PT,PU,PV,PW,PX,PY,PZ,QA,QB,QC,QD,QE,QF,QG,QH,QI,QJ,QK,QL,QM,QN,QO,QP,QQ,QR,QS,QT,QU,QV,QW,QX,QY,QZ,

ReferenceError: VD is not defined
    at /home/ubuntu/gopherjs/test.790433663:39:601154
    at Object.<anonymous> (/home/ubuntu/gopherjs/test.790433663:39:711135)
    at Object.<anonymous> (/home/ubuntu/gopherjs/test.790433663:51:4)
    at Module._compile (module.js:541:32)
    at Object.Module._extensions..js (module.js:550:10)
    at Module.load (module.js:458:32)
    at tryModuleLoad (module.js:417:12)
    at Function.Module._load (module.js:409:3)
    at Module.runMain (module.js:575:10)
    at run (node.js:348:7)
    at startup (node.js:140:9)
    at node.js:463:3
FAIL	reflect	0.418s

I guess running the same test without the minify option should give a more exploitable log. I should have a bit of time today to try that.

r-l-x · 2017-03-14T10:49:09Z

Here is the error without minify:

$ gopherjs test reflect
gopherjs: Source maps disabled. Use Node.js 4.x with source-map-support module for nice stack traces.
/home/rl/go/src/github.com/gopherjs/gopherjs/test.166851752:36502
	T1$1.init("reflect_test", [{prop: "T1inner", name: "", exported: true, typ: T1inner, tag: ""}, {prop: "f", name: "f", exported: false, typ: $Int, tag: ""}]);
	                                                                            ^

ReferenceError: T1inner is not defined
    at /home/rl/go/src/github.com/gopherjs/gopherjs/test.166851752:36502:78
    at Object.<anonymous> (/home/rl/go/src/github.com/gopherjs/gopherjs/test.166851752:37213:3)
    at Object.<anonymous> (/home/rl/go/src/github.com/gopherjs/gopherjs/test.166851752:43577:4)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:509:3
FAIL	reflect	0.386s

r-l-x · 2017-03-14T11:41:39Z

The only reference to a T1inner object I can find is in TestAlignment, which should be disabled in our case.

dmitshur · 2017-03-14T20:03:37Z

Yes, I'm not sure why, but in order to see the error with CircleCI, I need to fold and unfold the red part.

Thanks for the tip, that works for me.

The only reference to a T1inner object I can find is in TestAlignment, which should be disabled in our case.

It is indeed disabled from running that test, as you said. However, due to this change in this PR:

// Allow overridden function calls
// The standard library implementation of foo() becomes _gopherjs_overridden_foo()
if replacedDeclNames[funcName(d)] {
	d.Name = ast.NewIdent("_")
	d.Name.Name = "_gopherjs_overridden_" + d.Name.Name
}

It's no longer disabled from being compiled.

I suspect there may be a bug with deadcode elimination not handling this case well. /cc @neelance

However, I'm not able to reproduce it by doing something like this. Perhaps we should factor out that change and investigate it separately. We should also confirm that it doesn't have a significant negative effect on generated file sizes.

dmitshur · 2017-03-14T20:33:27Z

I've confirmed locally that's the case:

gopherjs $ gopherjs test --short reflect
PASS
ok  	reflect	3.742s
gopherjs $ open build/build.go
gopherjs $ git diff
diff --git a/build/build.go b/build/build.go
index 47a889e..260d242 100644
--- a/build/build.go
+++ b/build/build.go
@@ -308,7 +308,7 @@ func parseAndAugment(pkg *build.Package, isTest bool, fileSet *token.FileSet) ([
                        switch d := decl.(type) {
                        case *ast.FuncDecl:
                                if replacedDeclNames[funcName(d)] {
-                                       d.Name = ast.NewIdent("_")
+                                       d.Name.Name = "_gopherjs_overridden_" + d.Name.Name
                                }
                        case *ast.GenDecl:
                                switch d.Tok {
gopherjs $ go install -v
github.com/gopherjs/gopherjs/build
github.com/gopherjs/gopherjs
gopherjs $ gopherjs test --short reflect

/Users/Dmitri/Dropbox/Work/2013/GoLand/src/github.com/gopherjs/gopherjs/test.730446537:37375
})();
  ^
ReferenceError: T1inner is not defined
    at /Users/Dmitri/Dropbox/Work/2013/GoLand/src/github.com/gopherjs/gopherjs/test.730446537:36664:78
    at Object.<anonymous> (/Users/Dmitri/Dropbox/Work/2013/GoLand/src/github.com/gopherjs/gopherjs/test.730446537:37375:3)
    at Object.<anonymous> (/Users/Dmitri/Dropbox/Work/2013/GoLand/src/github.com/gopherjs/gopherjs/test.730446537:43739:4)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:422:7)
    at startup (bootstrap_node.js:143:9)
    at bootstrap_node.js:537:3
FAIL	reflect	0.659s
gopherjs $

dmitshur · 2017-03-14T21:04:57Z

Looking into it further, I think it may be a DCE/miscompilation issue introduced by that change itself. If so, that means it'll be more complicated to support overriding existing funcs than the 1 line change above.

r-l-x · 2018-04-19T13:47:18Z

Hello again. FYI i'm currently working to fix this by specifying which of the overridden functions should be kept, using a syntax in the function comments, like:

//gopherjs:keep_overridden
func foo() {
  ...
}

This is similar to the go generate directives.

dmitshur · 2018-04-19T22:14:01Z

About //gopherjs:keep_overridden, is it possible to defer that until a future PR?

A lot of code GopherJS already deals with preventing DCE by assigning to _ (search for "DCE" in the codebase). I worry it might be suboptimal to try to introduce a second mechanism in this PR, because then the project will be inconsistent about how it prevents DCE. That can be considered and applied in a followup PR.

Of course, if it's really unavoidable to make things work here, then it's ok.

r-l-x · 2018-04-20T09:43:21Z

Ok, I just pushed #798 for this. The branch associated to the current PR is rebased on the branch from #798, so the merge should be easy.

Before this commit, every overriden standard library function was renamed '_'. Now, if there is the following directive in the function comment: "//gopherjs:keep_overridden", they are prefixed with "_gopherjs_overridden_" so that they can be called from the overriding function. This is typically useful to use the standard library implementation as a fallback.

The overriding function tries to call the corresponding WebCryptoAPI function (which is much faster) when available, and fallbacks using the standard library implementation.

I don't see a way to implement crypto/ecdsa.Verify() based on webcrypto because the webcrypto implementation does include the hash where the go standard library interface doesn't. But this commit enables the webcrypto implementation for the cerificate signature checks made from the X.509 package.

This useful under Safari, when calling an unimplemented feature.

r-l-x · 2018-10-15T13:31:19Z

Hi, I just would like to have a feedback on this PR.
The DCE problems are now completely fixed by #798. If you are ready to merge, I'm happy to rebase both branches so that they are mergeable again. If there is something I need to fix, please let me know.

flimzy · 2021-12-28T12:42:08Z

@r-l-x Are you still interested in bringing this PR up to date for inclusion? #798 looks like it's close to being ready, which I understand would make this easier.

@nevkontakte were discussiong, and we think it might be wise to guard this functionality behind a build tag, at least initially, as well. We might want to discuss including that in this PR. But before we get too far down that road, we'd like to know if you're interested in pursuing this further.

Thanks.

r-l-x mentioned this pull request Dec 21, 2016

Slow crypto operations (WebCryptoAPI support) #554

Open

dmitshur reviewed Dec 21, 2016

View reviewed changes

flimzy reviewed Dec 21, 2016

View reviewed changes

flimzy reviewed Dec 22, 2016

View reviewed changes

flimzy mentioned this pull request Dec 23, 2016

proposal: add a way to hook into mainFinished #525

Closed

r-l-x changed the title ~~WIP: WebcryptoAPI support~~ WebcryptoAPI support Jan 3, 2017

flimzy reviewed Jan 9, 2017

View reviewed changes

neelance reviewed Jan 9, 2017

View reviewed changes

This was referenced Jan 27, 2017

Using native JS libs for improved compression performance #577

Open

Curious performance difference between callbacks and promises #579

Closed

r-l-x force-pushed the webcrypto-poc branch 3 times, most recently from c184f71 to a484d0e Compare July 20, 2017 13:36

r-l-x force-pushed the webcrypto-poc branch from a484d0e to e9763a3 Compare December 15, 2017 10:05

r-l-x force-pushed the webcrypto-poc branch from e9763a3 to 5cb19f4 Compare April 11, 2018 12:16

r-l-x force-pushed the webcrypto-poc branch from 5cb19f4 to 1a70f10 Compare April 20, 2018 09:18

r-l-x mentioned this pull request Apr 20, 2018

WIP: Ability to call overridden functions from the std library #798

Closed

r-l-x added 8 commits April 20, 2018 11:49

compiler/natives/src/crypto: Implement Sum256 based on webcrypto

9c0a841

The overriding function tries to call the corresponding WebCryptoAPI function (which is much faster) when available, and fallbacks using the standard library implementation.

compiler/natives/src/crypto/rand factorize Crypto interface access

fb758f0

webcrypto: added X.509 RSA certificate check

2b720aa

Updated test to allow importing github.com/gopherjs/gopherjs/js/*

b9ca915

webcrypto: catch JS exception during webcrypto calls

d0c66ed

This useful under Safari, when calling an unimplemented feature.

compiler/natives: Regenerate.

bcfda31

r-l-x force-pushed the webcrypto-poc branch from 1a70f10 to bcfda31 Compare April 20, 2018 09:58

r-l-x changed the title ~~WIP: WebcryptoAPI support~~ WebcryptoAPI support Apr 20, 2018

r-l-x changed the title ~~WebcryptoAPI support~~ WIP: WebcryptoAPI support Apr 21, 2018

flimzy added the NeedsHelp Community contributions are welcome for this feature! label Dec 28, 2021

WIP: WebcryptoAPI support #558

Are you sure you want to change the base?

WIP: WebcryptoAPI support #558

Uh oh!

Conversation

r-l-x commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Dec 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r-l-x Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitshur commented Dec 21, 2016

Uh oh!

flimzy Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r-l-x commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

r-l-x commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flimzy commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flimzy commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

r-l-x commented Dec 22, 2016

Uh oh!

flimzy commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

r-l-x commented Dec 22, 2016

Uh oh!

kothar commented Dec 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flimzy commented Dec 22, 2016

Uh oh!

dmitshur commented Dec 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

r-l-x commented Dec 27, 2016

Uh oh!

r-l-x commented Dec 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kothar commented Dec 29, 2016

Uh oh!

r-l-x commented Jan 9, 2017

r-l-x commented Dec 21, 2016 •

edited

Loading

r-l-x Dec 28, 2016 •

edited

Loading

dmitshur Dec 21, 2016 •

edited

Loading

flimzy Dec 21, 2016 •

edited

Loading

r-l-x commented Dec 22, 2016 •

edited

Loading

r-l-x commented Dec 22, 2016 •

edited

Loading

flimzy commented Dec 22, 2016 •

edited

Loading

flimzy commented Dec 22, 2016 •

edited

Loading

flimzy commented Dec 22, 2016 •

edited

Loading

dmitshur commented Dec 23, 2016 •

edited

Loading

r-l-x commented Dec 28, 2016 •

edited

Loading

r-l-x Jan 10, 2017 •

edited

Loading

r-l-x Jan 12, 2017 •

edited

Loading

lologarithm Feb 17, 2017 •

edited

Loading

r-l-x commented Mar 14, 2017 •

edited

Loading