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
Protect from null pointer exeption and silent failure #12308
Conversation
@@ -80,6 +80,11 @@ export class VisibilityModel { | |||
`Cannot repeat with interval less than ${MIN_REPEAT_INTERVAL}, ` + | |||
' repeat set to false'); | |||
} | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -80,6 +80,11 @@ export class VisibilityModel { | |||
`Cannot repeat with interval less than ${MIN_REPEAT_INTERVAL}, ` + | |||
' repeat set to false'); | |||
} | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI going to kill the whole if/else on line 65 next, and make repeat only be allowed to be a boolean, not a number. But probably won't get to that before the canary that's in 2 hours.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you might want to keep this PR simple for easy cherry-pick
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -80,7 +80,6 @@ export class VisibilityModel { | |||
`Cannot repeat with interval less than ${MIN_REPEAT_INTERVAL}, ` + | |||
' repeat set to false'); | |||
} | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
* Protect from null pointer exeption and silent failure * Revert log msg addition * Replace accidentally removed brace
* Protect from null pointer exeption and silent failure * Revert log msg addition * Replace accidentally removed brace
* Protect from null pointer exeption and silent failure * Revert log msg addition * Replace accidentally removed brace
@calebcordry Agreed. Just added to #12547.
https://github.com/ampproject/amphtml/pull/12547/files#diff-61c963752cc0a61e2ec5e1561dc0466cR217
…On Fri, Dec 22, 2017 at 12:40 PM, Caleb Cordry ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In extensions/amp-analytics/0.1/visibility-model.js
<#12308 (comment)>:
> @@ -239,8 +239,12 @@ export class VisibilityModel {
});
this.unsubscribe_.length = 0;
this.eventResolver_ = null;
- this.onTriggerObservable_.removeAll();
- this.onTriggerObservable_ = null;
+ // TODO(jonkeller): Investigate why dispose() can be called twice,
+ // necessitating this "if"
+ if (this.onTriggerObservable_) {
+ this.onTriggerObservable_.removeAll();
Should we also add warning here like in #12547
<#12547> ?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#12308 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACV2B6UD_IieX9oZldSwOGaJOdfwngFqks5tC-mcgaJpZM4Q2z1g>
.
|
I deleted the comment because I saw that you were one step ahead of me as soon as it was sent :) |
* Protect from null pointer exeption and silent failure * Revert log msg addition * Replace accidentally removed brace
Fixes #12304