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

Improve handling of switching dat file versions #8489

Closed
mrose17 opened this issue Apr 25, 2017 · 7 comments
Closed

Improve handling of switching dat file versions #8489

mrose17 opened this issue Apr 25, 2017 · 7 comments

Comments

@mrose17
Copy link
Member

mrose17 commented Apr 25, 2017

  • Did you search for similar issues before submitting this one? yes

  • Describe the issue you encountered: go to a site from the about:preference# payments page

  • Platform (Win7, 8, 10? macOS? Linux distro?): macOS

  • Brave Version (revision SHA): 6638b91

  • Steps to reproduce:

    1. run npm run watch & npm start
    2. go to about:preferences#payments
    3. click on site, e.g., coindesk.com
    4. BOOM
  • Actual result: crash

  • Expected result: new tab with website

  • Will the steps above reproduce in a fresh profile? If not what other info can be added?

  • Is this an issue in the currently released version? it's in master & dev-channel

  • Can this issue be consistently reproduced? yes

  • Extra QA steps: none

  • Screenshot if needed:
    0

  • Any related issues: none

@bbondy
Copy link
Member

bbondy commented Apr 25, 2017

@mrose17 try clearing your .dat files. I think this is a symptom of switching between dev-channel and master frequently. Let's leave this open though so I can improve that use case. Basically though your session stores the downloaded adblock version and then it's not actually that version.

@bbondy bbondy added this to the 0.15.1 milestone Apr 25, 2017
@bbondy bbondy changed the title FPE/divide by zero! Improve handling of switching dat file versions Apr 25, 2017
@bbondy bbondy self-assigned this Apr 25, 2017
@mrose17
Copy link
Member Author

mrose17 commented Apr 25, 2017

deleting the .dat files didn't help. i also removed the entire brave-development/ directory, started the browser, quit the browser, and put my ledger files back, and got a crash... wow!

@NejcZdovc
Copy link
Contributor

@mrose17 I had the same problem in #8486. What I did:

npm clean cache
rm -rf ~/.node-gyp
rm -rf ~/.electron
rm -rf node_modules

Cleared brave-development, git pull and then npm i.

Now it's working

@mrose17
Copy link
Member Author

mrose17 commented Apr 25, 2017

what?!? i don't have to reinstall either node or the OS?... (-;

the only step i was missing was the npm cache clean and getting rid of .node-gyp ... thanks!

@bbondy
Copy link
Member

bbondy commented Apr 26, 2017

actually I just thought of the reason I think you guys got this.

i) install node_modules on master you get ad-block library which expects data files in format v3.
ii) switch to dev-channel, but don't re-install node-modules. You have front end that looks for data format v2.

The opposite can happen too if you got he other way.

The way to fix this is to have the library specify the dat file version instead of having it separately defined in front end code.

@mrose17
Copy link
Member Author

mrose17 commented Apr 26, 2017

that makes sense. in this case, the culprit was .node_gyp which cached the ad-block library (i delete node_modules/ often...)

@bbondy
Copy link
Member

bbondy commented Apr 26, 2017

@bbondy bbondy closed this as completed in be2f2d3 Apr 26, 2017
bbondy added a commit that referenced this issue Apr 26, 2017
This adds a new argument version, which if undefined it uses the old way
to get it.  We only currently specify a version explicitly from the
ad-block lib, otherwise we use the front-end config.

The lib knows best which version it is on. In this repo we had problems
because people switch between branches but don't npm install again.
With a fully C++ implementation this wouldn't have been seen.

Related: brave-experiments/ad-block@85e6b35

Fix #8489

Auditors: @SergeyZhukovsky
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.