Skip to content

Fix possible panic during internalization/externalization of time.Time/Date. #285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 31, 2015

Conversation

dmitshur
Copy link
Member

Begin by adding a failing test for issue #279.

Proper testing for this issue prevents "testing" package from being imported. So, implement it as a low level single file .go test and invoke it from backend. (See #284 for related context.)

In the time_inexternalization.go, you can use these statements for
manual testing:

var _ = time.Now()      // Force time.Time not to be DCEed.
var _ = time.Unix(0, 0) // Force time.Unix (and time.Time implicitly) not to be DCEed.

Adding both of those statements (or just time.Unix one, since it also includes time.Time indirectly) would cause the test to pass. But a true fix will be in the compiler itself, in a way that will avoid the panics/incorrect behavior even when time.Time and/or time.Unix are DCEed, or time package is not imported at all.

Edit: Implemented fix for the issue, so it now resolves #279.

@dmitshur
Copy link
Member Author

Test is currently failing with:

lowlevel_test.go:20: exec: "gopherjs": executable file not found in $PATH:

@neelance, since you're more familiar with Circle CI (I am more familiar with Travis), do you know how we can make it so the compiled gopherjs binary is added to PATH?

Edit: This has been resolved in cf3fd3a.

@dmitshur dmitshur mentioned this pull request Aug 30, 2015
@dmitshur
Copy link
Member Author

I've implemented a fix for issue #279 that successfully resolves the failing test, please see and review #286 which is set to merge into this PR.

@neelance
Copy link
Member

Looks like a good test for the issue. LGTM

An empty .go file is needed because this package is empty aside from
tests. Rename it to doc.go since that's a more common name and it
becomes more readable at a glance (e.g., people can know it will not
contain any code without even having to open/look in it).
…ime/Date.

This is a test for issue #279.

Proper testing for this issue prevents "testing" package from being
imported. So, implement it as a low level single file .go test and
invoke it from backend.

In the time_inexternalization.go, you can use these statements for
manual testing:

	var _ = time.Now()      // Force time.Time not to be DCEed.
	var _ = time.Unix(0, 0) // Force time.Unix (and time.Time implicitly) not to be DCEed.

Adding both of those statements (or just time.Unix one, since it also
includes time.Time indirectly) would cause the test to pass. But a true
fix will be in the compiler itself, in a way that will avoid the
panics/incorrect behavior even when time.Time and/or time.Unix are
DCEed, or time package is not imported at all.
This allows tests to invoke gopherjs binary when needed.

According to https://circleci.com/docs/environment-variables, $PATH
includes /home/ubuntu/bin and $HOME is /home/ubuntu.
…e/Date.

Fixes #279 (initial report).

The internalization/externalization code only checks if the time
package is present and assumes time.Unix and time.Time are available if
so. Previously, it was possible for time package to be present, but
either time.Unix or time.Time were not (due to dead code elimination),
causing possible panics.

In time package itself, ensure time.Unix func and time.Time struct it
returns are always included (despite DCE) to prevent that from
happening.
@dmitshur dmitshur force-pushed the fix-timeTime-Date-inexternalization branch from d0cc457 to 6e0c1e9 Compare August 30, 2015 20:47
@dmitshur
Copy link
Member Author

This is ready to merge now and resolves the issue fully. LGTM.

Fixes #279 (additional findings).

When time package is not imported, it's still possible to internalize a
Date object (into an interface{}). Due to fallthrough being default,
this resulted in Function case running when internalizing Date with no
time package.

That makes no sense, and means that trying to externalize the Date
object back into JavaScript world would fail to produce the original
Date object (also, calling methods on it would obviously fail, since
it's not a real Date object).

For example, consider the following code that doesn't import time
package, it can now run and produce expected results:

	date := js.Global.Get("Date").New().Interface()
	js.Global.Get("console").Call("log", date)
Apply feedback from
#286 (comment).
@dmitshur dmitshur force-pushed the fix-timeTime-Date-inexternalization branch from 6e0c1e9 to 8bffb58 Compare August 31, 2015 09:17
@dmitshur
Copy link
Member Author

(I've reworded the second last commit to have a slightly better commit message. It went from "Internalize Date when time package not imported in a reasonable way." to "Internalize Date in a reasonable way when time package not imported.")

dmitshur added a commit that referenced this pull request Aug 31, 2015
…tion

Fix possible panic during internalization/externalization of time.Time/Date.
@dmitshur dmitshur merged commit 23bfde2 into master Aug 31, 2015
@dmitshur dmitshur deleted the fix-timeTime-Date-inexternalization branch August 31, 2015 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with time.Sleep
2 participants