Skip to content
This repository has been archived by the owner. It is now read-only.

Browser crash with payment enabled when clearing browsing data #8670

Merged
merged 4 commits into from May 5, 2017
Merged

Conversation

@mrose17
Copy link
Member

mrose17 commented May 4, 2017

Test Plan:

  1. start browser
  2. oh, the places you will go!
  3. clear all browser data via preferences>security
  4. no crash: winner!

Description

40abc0c
065d1c784268b47#diff-8a9bac13ef6264feb2b6da7c18d86b3cL733 and
40abc0c
065d1c784268b47#diff-8a9bac13ef6264feb2b6da7c18d86b3cL746 you’ll see
that the old code had a catch to deal with clean-up errors. The new
code doesn’t. This fixes that in filtering.js … whether or not there
should be a catch is a different issue.

Also, in tabs.js, I consistently get a crash when starting up my test
browser, this fixes that.

Fixes #8659 ... this has nothing to do with the ledger, afaik...

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
40abc0c
065d1c784268b47#diff-8a9bac13ef6264feb2b6da7c18d86b3cL733 and
40abc0c
065d1c784268b47#diff-8a9bac13ef6264feb2b6da7c18d86b3cL746 you’ll see
that the old code had a catch to deal with clean-up errors. The new
code doesn’t. This fixes that in filtering.js … whether or not there
should be a catch is a different issue.

Also, in tabs.js, I consistently get a crash when starting up my test
browser, this fixes that.
@mrose17 mrose17 requested a review from bridiver May 4, 2017
@luixxiul luixxiul added this to the 0.15.3 milestone May 4, 2017
@bridiver
Copy link
Collaborator

bridiver commented May 4, 2017

actually it looks like the catch was hiding the fact that the method was always throwing an exception. You're right that we don't want to add a catch, we want to fix the method call so it doesn't throw an exception every time

@mrose17
Copy link
Member Author

mrose17 commented May 4, 2017

@bridiver - i agree, but this is outside of my area, so i'm deferring to you, @bsclifton and @bbondy to figure out the rea fix...

mrose17 added 2 commits May 4, 2017
@bsclifton
Copy link
Member

bsclifton commented May 4, 2017

@bridiver before I can test this out, I need your help resolving the merge conflict. It conflicts with the commit you did most recently: 33e3fca

@mrose17
Copy link
Member Author

mrose17 commented May 4, 2017

@bsclifton -please resolve the conflicts in favor of my PR. it supersedes the checkin done by @bridiver

the only difference is that i added a .bind(ses) to what @bridiver did...

@alexwykoff
Copy link
Contributor

alexwykoff commented May 5, 2017

need to get to stable release so needs to be finished todayish, also needs test plan

@bbondy bbondy changed the title If you look at... Browser crash with payment enabled when clearing browsing data May 5, 2017
@mrose17
Copy link
Member Author

mrose17 commented May 5, 2017

@bsclifton - conflicts resolved.

Copy link
Member

bsclifton left a comment

++

@bsclifton
Copy link
Member

bsclifton commented May 5, 2017

awesome, thanks @mrose17 😄

Per request from @alexwykoff, do you mind adding a test plan?

@mrose17
Copy link
Member Author

mrose17 commented May 5, 2017

@bsclifton - done.

@mrose17 mrose17 merged commit 32ed15b into master May 5, 2017
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@mrose17 mrose17 deleted the issue-8659 branch May 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.