cmd+click on some PDFs no longer opens them in Brave

[LaurenWags]

Description

In past versions (0.23.31, 0.23.39, and even 0.23.70 which was the previous c68 build), if you cmd+click (or right click + open in new tab option) on the BAT White Paper PDF, it would open in a new tab. In 0.23.72 that no longer happens. Note - if you click on the link/button directly you are prompted to download the white paper, cmd+click allowed you to view the PDF in Brave without downloading.

Steps to Reproduce

1. Navigate to Manual test run on OS X for 0.23.x Release 4 w/ Chromium 68 (BETA Channel) #14838 and go to Content Tests section.
2. Locate the link for BAT White Paper PDF.
3. cmd+click (mac) on the link.
4. If you watch carefully, you can see that a new tab opens to the right of the manual test run, but quickly closes.
5. Try right click + open in new tab from context menu. Same result.
6. Try right click + open in new window from context menu. Blank window opens.
7. Navigate to basicattentiontoken.org
8. Repeat steps 3-6 on the BAT White Paper button.

Actual result:
PDF no longer opens in Brave: (note - opened tab disappears very quickly and is not captured in the gif below)

Expected result:
PDF should open in Brave as it did in previous versions (I checked 0.23.70, 0.23.39, 0.23.31)

Reproduces how often:
easily

Brave Version

about:brave info:
0.23.72

Reproducible on current live release:
no

Additional Information

Maybe related to #14409?

Also, if you try the STR on the pdf995 link, this issue does not occur, so it's only seems like it happens for some PDFs.

Reproduced by @kjozwiak

[darkdh]

I can't reproduce this. @kjozwiak @srirambv can you try?

[srirambv]

I can reproduce on Windows too. But if there is a site opened in a new tab next to the source tab and then Ctrl+click on the link saves the file. But this cant be reproduced all the time

[btlechowski]

Reproducible on Linux v0.23.72

Recently there was an issue fixed: #10554, which changed the handling of pdfs based on content-type.
Maybe the pdf is recognized as regular file to be download and what we see is: #14409

In Chrome the pdf is loaded in the browser.

[bsclifton]

I believe this is a duplicate of #14409

[LaurenWags]

@bsclifton it could be, but the steps in the description worked in 0.23.31, 0.23.39 and even 0.23.70 just as an FYI.

Some background - I was using the steps in the description to be able to load the PDF in the browser because clicking on the link directly forces you to download the PDF (which is not what I wanted, I wanted to view it in Brave). So right now, I can't find any way to get the PDF listed to open in Brave without downloading. (note - this is not an issue in Chrome, clicking on the link in Chrome allows it to open without having to cmd+click on the link)

[diracdeltas]

likely related to #13587
since #8364 is the original issue that the reverted code was fixing

[diracdeltas]

"If you watch carefully, you can see that a new tab opens to the right of the manual test run, but quickly closes." < I just repro'ed this on Mac and it's not what happened in #8364. So i think this is a new issue caused somewhere in muon.

[diracdeltas]

easy way to repro is to go to https://basicattentiontoken.org/ and just click the "view the white paper" button. you don't even need to cmd+click

[bsclifton]

If I spam click as fast as I can, I am seeing this get logged:

[64269:15127:0726/154137.507578:ERROR:mach_port_broker.mm(175)] Unknown process 64295 is sending Mach IPC messages!
[64269:15127:0726/154139.122244:ERROR:mach_port_broker.mm(175)] Unknown process 64298 is sending Mach IPC messages!
[64269:15127:0726/154139.332268:ERROR:mach_port_broker.mm(175)] Unknown process 64299 is sending Mach IPC messages!
[64269:15127:0726/154141.571418:ERROR:mach_port_broker.mm(175)] Unknown process 64300 is sending Mach IPC messages!
[64269:15127:0726/154141.717987:ERROR:mach_port_broker.mm(175)] Unknown process 64301 is sending Mach IPC messages!
[64269:15127:0726/154141.870061:ERROR:mach_port_broker.mm(175)] Unknown process 64303 is sending Mach IPC messages!

Agreed that it's likely in Muon- will be investigating

[bsclifton]

with the --debug-tab-events debug flag, we get the following when opening the white paper PDF in a tab:

Creating tab with properties:  { url: 'https://basicattentiontoken.org/BasicAttentionTokenWhitePaper-4.pdf',
  openerTabId: 3,
  active: false,
  autoDiscardable: true }
Tab [5] created in window -1
Contents [5] process:add-new-contents { userGesture: true,
  isBackground: false,
  disposition: 'background-tab' }
notifyWindowWebContentsAdded: on tab creation in existing window 2
Tab [5] event 'tab-inserted-at'
Tab [5] event 'render-view-created'
Tab [5] event 'did-start-loading'
Tab [5] event 'load-progress-changed'
[5] load-progress: 0.1
Tab [5] event 'did-start-navigation'
Tab [5] event 'load-start'
Tab [5 Updating state index from -1 to 3
updateTabIndexesForWindow took: 0s 0.642885ms with 1 changes
Tab [5] event 'did-finish-navigation'
tab [5 via process] chrome-tabs-updated { status: 'complete', url: '' }
Tab [5] updated from muon. changeInfo: { status: 'complete', url: '' } currentValues: { newIndex: 3,
  newActive: false,
  windowId: 2,
  isPlaceholder: false,
  guestInstanceId: 5 }
Tab [5] event 'did-fail-provisional-load'
Tab [5] event 'load-progress-changed'
[5] load-progress: 1
Tab [5] event 'did-stop-loading'
Tab [5] will-download
Tab [5] event 'close'
Tab [5] event 'render-view-deleted'
Tab [5] event 'render-view-deleted'
Tab [5] event 'devtools-closed'
Tab [5] event 'will-destroy'
tab [5 via process] - chrome-tabs-removed
tab 5 will be removed from window 2, wasActive: false
Tab [probably 5] event 'destroyed'
[64710:145691:0726/155317.609243:ERROR:mach_port_broker.mm(175)] Unknown process 64786 is sending Mach IPC messages!
updateTabIndexesForWindow took: 0s 1.15966ms with 0 changes
Tab [5] forgetTab: is already destroyed, cleaning up webContents from cache immediately
updateTabIndexesForWindow took: 0s 0.65326ms with 0 changes
Tab [3] event 'update-target-url'
Tab [3] frame changed for tab

The will-download with nothing happening seems suspicious. When performing the steps from #14409, the output is identical (which is why I think it may be the same issue). Will keep digging here

[bsclifton]

DownloadItem::GetSavePath() is returning empty string which is causing the download to kick out. Will need to add some debugging to Muon

[LaurenWags]

@bsclifton the steps in the issue worked as expected in muon 8.0.1 (with b-l build 0.23.70) and did not work in 8.0.2 (with b-l build 0.23.72) if that helps. If I can help out at all please let me know 😄

[bsclifton]

Weird... if you turn off Always ask me where to save files it seems to actually download

Here's the code path it's hitting (which cancels the download):
https://github.com/brave/muon/blob/4ffc7f014f0b8cc8853fb39e2efe35e30194abbe/atom/browser/atom_download_manager_delegate.cc#L419-L425

[bsclifton]

It's hitting this condition because the webview is not attached to the webcontents. When it does the lookup (code link above), it fails and exits

[bsclifton]

@LaurenWags this behavior likely changed with #13587

After this was introduced, I believe this now duplicates #14409

[diracdeltas]

@bsclifton i'm still seeing this issue on latest release. right-click to open PDF in a new tab just shows the tab for an instant and closes it. it doesn't download the PDF or show it.

[diracdeltas]

closing bc i just saw brave/muon@4504af1

[bsclifton]

@diracdeltas did you test with 0.23.74?
https://github.com/brave/browser-laptop/releases/tag/v0.23.74dev

It's very possible we need to re-open this issue because now it seems to prompt to save, instead of opening in browser. As you mentioned above, this change (open vs download) may have been caused by #13587

I closed it since it was basically no-oping (which matches the behavior of #14409, which as you noted was fixed by brave/muon@4504af1). But if we'd like this to OPEN the PDF, we can remove the duplicate label and re-open