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

PDFJS returns CORS error when PDF is loaded directly #2715

Closed
diracdeltas opened this issue Jul 25, 2016 · 5 comments · Fixed by #12138
Closed

PDFJS returns CORS error when PDF is loaded directly #2715

diracdeltas opened this issue Jul 25, 2016 · 5 comments · Fixed by #12138
Assignees
@diracdeltas
Labels
feature/extensions plugin/pdfjs QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/include
Milestone
0.20.x (Release C...

Comments

@diracdeltas
Copy link
Member

diracdeltas commented Jul 25, 2016 •
edited by luixxiul
Loading

Test plan

#12138 (comment)

611f617 works around the fact that a PDF loaded directly in Brave via webview loadUrl causes a CORS error. reloading always fixes the error for me. seems like some kind of race condition with how the origin is being set.
screen shot 2016-07-25 at 6 20 05 pm
diracdeltas referenced this issue Jul 25, 2016
@diracdeltas
Fix PDFJS MIME type detection
611f617 
This reverts the old workaround for pdfjs returning status code 0 errors,
which was causing PDF MIME type detection to stop working in cases
where the URL is loaded directly instead of by navigation. A webview
reload seems to always get rid of the PDF status code 0 error.

This includes a merge of PDFJS latest master, but the fix commit is only
diracdeltas/pdf.js@6c18572

Fix #2633
Fix #2654
Fixes the session tab part of #2568

Auditors: @bbondy
@Rob--W
Copy link

Rob--W commented Jul 26, 2016

Why is the commit called "Fix PDFJS MIME type detection"? I tried hard, but couldn't relate that commit description to the code that had been committed.

I didn't look at Brave's source code, but I know that in Chrome there is an issue with granting host permissions (=setting up the origin whitelist in the renderer for the given frame) when an extension is being loaded in a data:-URL. For instance, when PDF.js is embedded as follows: data:text/html,<iframe src="chrome-extension://oemmndcbldboiebfnladdacbdfmadadm/https://bitcoin.org/bitcoin.pdf">, then the same error as in your screenshot is shown. This is not PDF.js-specific, any extension suffers from it.

@diracdeltas
Copy link
Member Author

@Rob--W originally, to work around this CORS issue, I added a shortcut in Brave to bypass PDF.JS's onHeadersReceived PDF mime type detection: 00a70f5. This turned out to be what was causing a bunch of 'Malformed PDF' bugs that we were seeing, including #2654.

The CORS issue is still a mystery, but it seems to always go away when the viewer URL is reloaded, so that's the workaround for now.

@diracdeltas
Copy link
Member Author

diracdeltas commented Jul 26, 2016 •
edited
Loading

it's worth noting the CORS error never appears when a PDF is navigated to, such as by clicking a link. it seems to only fire when webview.loadURL (http://electron.atom.io/docs/api/web-view-tag/) is called. calling webview.reload() fixes it.

diracdeltas referenced this issue in brave/muon Aug 1, 2016
@bridiver
enable web request for incognito contexts
2138301 
auditors: @bbondy, @diracdeltas
@diracdeltas
Copy link
Member Author

diracdeltas commented Aug 1, 2016 •
edited
Loading

the webview.reload() workaround no longer works as of brave/muon@02ec0b2

@diracdeltas diracdeltas modified the milestones: 0.11.3dev, 1.0.0, 0.11.2dev Aug 1, 2016
diracdeltas added a commit to brave/muon that referenced this issue Aug 1, 2016
@diracdeltas
Don't suppress renderer restart for chrome-extension
7e61ac2 
The renderer process needs to be restarted for PDFJS to load PDFs without
encountering a CORS error. Brave was calling webview.reload in order to force
renderer process restart on non-navigation PDF loads, but this was broken by
02ec0b2. This commit fixes it but
a better solution would be for renderer reload to automatically happen
on non-navigation chrome-extension loads.

cf brave/browser-laptop#2715

Auditors: @bridiver
@diracdeltas
Copy link
Member Author

workaround fixed in brave/muon@7e61ac2

@diracdeltas diracdeltas modified the milestones: 0.11.3dev, 0.11.2dev Aug 1, 2016
@bbondy bbondy modified the milestones: 0.11.3dev, 0.11.4dev, 0.11.5dev Aug 4, 2016
@bbondy bbondy modified the milestones: 0.11.5dev, 0.11.6dev Aug 12, 2016
bridiver pushed a commit to brave/muon that referenced this issue Aug 15, 2016
@diracdeltas @bridiver
Don't suppress renderer restart for chrome-extension
7c91453 
The renderer process needs to be restarted for PDFJS to load PDFs without
encountering a CORS error. Brave was calling webview.reload in order to force
renderer process restart on non-navigation PDF loads, but this was broken by
02ec0b2. This commit fixes it but
a better solution would be for renderer reload to automatically happen
on non-navigation chrome-extension loads.

cf brave/browser-laptop#2715

Auditors: @bridiver
@bbondy bbondy modified the milestones: 1.0.0, 0.11.6dev Aug 23, 2016
bridiver pushed a commit to brave/muon that referenced this issue Sep 17, 2016
@diracdeltas @bridiver
Don't suppress renderer restart for chrome-extension
bc62fab 
The renderer process needs to be restarted for PDFJS to load PDFs without
encountering a CORS error. Brave was calling webview.reload in order to force
renderer process restart on non-navigation PDF loads, but this was broken by
02ec0b2. This commit fixes it but
a better solution would be for renderer reload to automatically happen
on non-navigation chrome-extension loads.

cf brave/browser-laptop#2715

Auditors: @bridiver
@alexwykoff alexwykoff modified the milestones: 1.0.0, Backlog Nov 1, 2017
diracdeltas added a commit that referenced this issue Nov 29, 2017
@diracdeltas
move pdf.js loading logic into browser-laptop
498bc75 
fix #4651
probably fixes #2715 but ideally the workaround in brave/pdf.js should be removed too
may also fix other PDF loading errors
@diracdeltas diracdeltas mentioned this issue Nov 29, 2017
Merged
8 tasks
bsclifton pushed a commit that referenced this issue Dec 19, 2017
@diracdeltas @bsclifton
move pdf.js loading logic into browser-laptop
68f188f 
fix #4651
probably fixes #2715 but ideally the workaround in brave/pdf.js should be removed too
may also fix other PDF loading errors
diracdeltas added a commit that referenced this issue Dec 19, 2017
@diracdeltas
move pdf.js loading logic into browser-laptop
ddf13d6 
fix #4651
probably fixes #2715 but ideally the workaround in brave/pdf.js should be removed too
may also fix other PDF loading errors
@bsclifton bsclifton modified the milestones: Triage Backlog, 0.20.x (Beta Channel) Dec 27, 2017
This was referenced Dec 27, 2017
Closed
Closed
Closed
This was referenced Jan 2, 2018
Closed
Closed
@srirambv srirambv mentioned this issue Jan 9, 2018
Closed
29 tasks
@LaurenWags LaurenWags mentioned this issue Jan 22, 2018
Closed
34 tasks
@srirambv srirambv mentioned this issue Jan 24, 2018
Closed
@kjozwiak kjozwiak mentioned this issue Jan 31, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@diracdeltas diracdeltas

Labels
feature/extensions plugin/pdfjs QA/checked-Linux QA/checked-macOS QA/checked-Win64 QA/test-plan-specified release-notes/include
Projects
None yet
Milestone
0.20.x (Release Channel)
Development

Successfully merging a pull request may close this issue.

move pdf.js loading logic into browser-laptop brave/browser-laptop
9 participants
@diracdeltas @bbondy @Rob--W @alexwykoff @kjozwiak @luixxiul @bsclifton @srirambv @LaurenWags