brave / brave-browser

Brave browser for Android, iOS, Linux, macOS, Windows.
https://brave.com
Mozilla Public License 2.0
17.85k stars 2.34k forks source link

pinned PDF tab imported from muon not filtered out - follow up to 2361 #2376

Closed LaurenWags closed 5 years ago

LaurenWags commented 5 years ago

Description

Found while testing #2361 (see https://github.com/brave/brave-browser/issues/2361#issuecomment-444585333)

If you have about pages or PDFs those should not be imported into b-c per 2361. However, during testing I found that if a PDF is pinned, it is imported to b-c.

Steps to Reproduce

  1. Open Muon and create the following tabs: a. A normal tab that resolves to an external URL, e.g. https://brave.com b. A normal tab that resolves to an internal URL with the about: scheme, e.g. about:brave c. A normal tab that resolves to an internal URL with the chrome-extension:// scheme, e.g. https://brave.com/Brave_mobile_speedtests_July.pdf.
  2. Repeat step 1, creating pinned tabs instead of normal tabs.
  3. Import these open windows/tabs from Muon into brave-core (either with --upgrade-from-muon flag on first launch or manually through Import)

Actual result:

Pinned PDF tab is imported into b-c, but the opened (non-pinned) PDF tab is not. image

Expected result:

Open tabs resolving to an external URL should be imported into brave-core, while open tabs resolving to an internal URL (about pages and PDFs) should not be imported into brave-core.

Reproduces how often:

easily

Brave version (brave://version info)

Brave 0.57.16 Chromium: 71.0.3578.80 (Official Build) (64-bit)
Revision 2ac50e7249fbd55e6f517a28131605c9fb9fe897-refs/branch-heads/3578@{#860}
OS Mac OS X

Reproducible on current release:

Website problems only:

Additional Information

cc @garrettr @bsclifton @rebron

btlechowski commented 5 years ago

On Windows 7, a prompt is shown to save the pdf file. Nevertheless the pdf link is filtered out of tabs and pinned tabs.

Tested on

Brave 0.57.17 Chromium: 71.0.3578.80 (Official Build) (64-bit)
Revision 2ac50e7249fbd55e6f517a28131605c9fb9fe897-refs/branch-heads/3578@{#860}
OS Windows
garrettr commented 5 years ago

This is because Muon uses a different location for PDFs depending on whether they are part of a normal tab or a pinned tab. Pinned tabs have a location that refers to the actual URL (e.g. "location": "https://brave.com/Brave_mobile_speedtests_July.pdf") while normal tabs have a location that refers to the pdf.js extension (e.g. "location": "chrome-extension://jdbefljfgobbmcidnmpjamcbhnbphjnb/https://brave.com/Brave_mobile_speedtests_July.pdf").

Since we are filtering based on the scheme, the current filtering works for locations that start with chrome-extension but not others.

If we really want to avoid loading any PDF tabs, one option is to also filter on extension and just drop all locations that have a .pdf extension.

srirambv commented 5 years ago

On Linux none of the pinned tabs are imported. I had the following tabs pinned Pinned tab 1: PDF Link Pinned tab 2: brave.com Pinned tab 3: about://passwords Pinned tab 4: about://extensions Pinned tab 5: about://preferences#payments

Upon import on 0.57.17, no new window was imported and no pinned tab for brave.com was also created

garrettr commented 5 years ago

I'm waiting on Linux and Windows builds now and will investigate once they're done..

bsclifton commented 5 years ago

Closing as wontfix