Skip to content
This repository has been archived by the owner on Apr 3, 2019. It is now read-only.

fix(server): fix undefined dereference #6086

Merged
merged 1 commit into from Apr 20, 2018
Merged

fix(server): fix undefined dereference #6086

merged 1 commit into from Apr 20, 2018

Conversation

philbooth
Copy link
Contributor

@philbooth philbooth commented Apr 19, 2018

Fixes #6085, maybe.

Sentry says:

TypeError: parent.toVersionString is not a function
  File "/app/node_modules/express/lib/router/index.js", line 284, in null.<anonymous>
    trim_prefix(layer, layerError, layerPath, path);
  File "/app/node_modules/express/lib/router/index.js", line 335, in Function.process_params
    return done();
  File "/app/node_modules/express/lib/router/index.js", line 275, in next
    self.process_params(layer, paramcalled, req, res, function (err) {
  File "/app/node_modules/express/lib/router/index.js", line 174, in Function.handle
    next();
  File "/app/node_modules/express/lib/application.js", line 174, in EventEmitter.handle
    router.handle(req, res, done);
...
(5 additional frame(s) were not displayed)

This change should fix that, but there are no tests because I couldn't come up with a failing test case.

I tried every possible user-agent string I could think of to reproduce it, without success. Sentry says the failing user-agent string was this:

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:59.0) Gecko/20100101 Firefox/59.0

Which surprised me and, sure enough, that works fine locally. I also tried prematurely terminating the string, inserting nulls, newlines, mismatched parentheses, specifying the empty string, strings that only contained whitespace, emojis and other deliberately weird inputs. None of them reproduce the behaviour for me.

So on the one hand, Sentry is really helpful. On the other hand, sometimes I get the impression that it isn't always truthful.

Was the stack trace for this error correct? The user-agent string? Was there really an error?

No idea, but hopefully this makes it go away.

@mozilla/fxa-devs r?

@ghost ghost assigned philbooth Apr 19, 2018
@ghost ghost added the waffle:active label Apr 19, 2018
@@ -34,7 +34,7 @@ function safeFamily (parent) {
}

function safeVersion (parent) {
if (! VALID_VERSION.test(parent.toVersionString())) {
if (parent && parent.toVersionString && ! VALID_VERSION.test(parent.toVersionString())) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's super pedantic, do you want to check whether toVersionString is a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope.

Copy link

@shane-tomlinson shane-tomlinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+

@philbooth philbooth merged commit dc6e30b into train-110 Apr 20, 2018
@ghost ghost removed the waffle:active label Apr 20, 2018
@philbooth philbooth deleted the pb/6085 branch April 20, 2018 06:53
divyabiyani pushed a commit to divyabiyani/fxa-content-server that referenced this pull request Apr 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants