New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 776661 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Not on Chrome anymore
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Regression : An added "PDF Viewer" extension doesn't get applied to certain PDF pages.

Reported by avsha...@etouch.net, Oct 20 2017

Issue description

Chrome version : 64.0.3245.0 (Official Build) 140f8ce04cc4ee5d4b3ca2eb83888d2452a1f43b-refs/heads/master@{#510270} 32/64 bit
OS : Windows(7,8,10), Linux(14.04 LTS), Mac(10.12.6)

Test URL 1 : https://chrome.google.com/webstore/detail/pdf-viewer/oemmndcbldboiebfnladdacbdfmadadm?utm_source=chrome-ntp-icon

Test URL 2 : https://msu.edu/~urban/sme865/resources/embedded_pdf.html

What steps will reproduce the problem?
1. Launch chrome, navigate to test URL 1 and install an "PDF Viewer" extension.
2. Now open NTP and navigate to test URL 2 (let the page load completely).
3. Scroll down the page and observe the PDF.

Actual Result : The "PDF Viewer" extension doesn't get applied to certain PDF pages.

Expected Result : Added "PDF Viewer" extension should get applied to all PDF pages.

This is a regression issue broken in ‘M-64’ and using the per-revision bisect providing the bisect results,
Good build : 64.0.3241.0 (Revision : 508935)
Bad build : 64.0.3242.0 (Revision : 509211)

You are probably looking for a change made after 509093 (known good), but no later than 509095 (first known bad).

CHANGELOG URL: 
https://chromium.googlesource.com/chromium/src/+log/0da0da31394b2137d63e0f9697de13f2453ff833..e4488e203ff33223f228dd2359383d2085a76c07

Suspect : https://chromium.googlesource.com/chromium/src/+/e4488e203ff33223f228dd2359383d2085a76c07

Note : Above issue can also be reproduced on another URL - https://pdfobject.com/

@sky : Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.
 
Actual_PDF.mp4
1.9 MB View Download
Expected_PDF.mp4
1.5 MB View Download
Labels: ReleaseBlock-Stable
Tagging with blocker label, please undo if not the case.

Comment 2 by sky@chromium.org, Oct 20 2017

Cc: kenrb@chromium.org
Owner: wjmaclean@chromium.org
I reverted my patch locally and it didn't impact this (nor would I have expected it to). I suspect you have the regression range wrong. I've passing to wjmaclean, maybe he has an idea as to change might have broken this.
Cc: ekaramad@chromium.org creis@chromium.org
+ekaramad@ who knows about PDF loading, and
+creis@ since this looks like a possible navigation issue?
I don't think it is MimeHandlerView related. I think the extension redirects the chrome-extension://kPdfExtensionId/index.html?pdf to its own url [1]. So maybe the webRequest.onBeforeRequest is failing?

1- https://github.com/mozilla/pdf.js/blob/master/extensions/chromium/extension-router.js
Components: -Internals>Plugins>PDF
(Yet again) Not in the internal PDF viewer -> Internals>Plugins>PDF is the wrong label.

Comment 6 by creis@chromium.org, Oct 20 2017

Cc: rdevlin....@chromium.org rob@robwu.nl wjmaclean@chromium.org nasko@chromium.org
Labels: Needs-Bisect
Owner: rdevlin....@chromium.org
Maybe there was a webRequest change recently?  Devlin, could you help triage?

I'm requesting another bisect since the CLs in the regression range don't seem right.  Could be related to a field trial.  (I've verified it's not the --site-per-process trial which is going on this week, since I can repro the bug on 64.0.3245.0 with or without that flag.)
Owner: karandeepb@chromium.org
I'm not aware of any webRequest changes recently.  Presumably this wouldn't be PlzNavigate related, either?

karandeepb@, any ideas?
Again, not really aware of any webRequest changes. Will wait for another bisect.

Comment 9 by avsha...@etouch.net, Oct 23 2017

Cc: karandeepb@chromium.org m.jeth...@eyeo.com
Labels: -Needs-Bisect
With response to C#6,

Performed another bisect and got different results: 

CHANGELOG URL: 
https://chromium.googlesource.com/chromium/src/+log/e4488e203ff33223f228dd2359383d2085a76c07..30168b99604c48a32828f26b5acb2674beea1a5d

Suspect : https://chromium.googlesource.com/chromium/src/+/30168b99604c48a32828f26b5acb2674beea1a5d

@Manish : Could you please look into the issue.

Comment 10 by m.jeth...@eyeo.com, Oct 23 2017

I reverted my change and tried out the extension, it is still broken.

I looked at the extension's code: It injects one CSS file into the content script [1], and while this could have been the problem, it doesn't seem like this is the case.

[1]: https://github.com/mozilla/pdf.js/blob/d9f90d595d0eb64875e9d3ed252ae0d0c2aca2bf/extensions/chromium/manifest.json#L28

Comment 11 by m.jeth...@eyeo.com, Oct 23 2017

Well hold on ... I may have made a mistake, it does seem to work after I revert my change. I'm not sure if this is intermittent or what. Let me investigate.

Comment 12 by m.jeth...@eyeo.com, Oct 23 2017

This is definitely a regression caused by 30168b9

The extension uses the "animationstart" event to detect the presence of embedded PDFs. Animations in injected style sheets are no longer working.
Cc: r...@opera.com
Owner: nainar@chromium.org
As per c#12, un-assigning myself and assigning to nainar@ for further triage (Can't assign to m.jethani@ and rune@ doesn't seem to be in use any more).
Yup, rune@opera.com isn't in use anymore. 

m.jethani@ this issue is on Canary so feel free to take some time to look into the issue. Ideally we should have this issue resolved in time for branch which is late November. If you want to initiate a revert let me know here and I can initiate it for you. 

Comment 15 by m.jeth...@eyeo.com, Oct 24 2017

I've uploaded a patch:

https://crrev.com/c/735263
Project Member

Comment 16 by bugdroid1@chromium.org, Oct 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bed9cb9d56adac6f50313b0e620a1aa2f0d871ed

commit bed9cb9d56adac6f50313b0e620a1aa2f0d871ed
Author: Manish Jethani <m.jethani@eyeo.com>
Date: Sat Oct 28 23:02:07 2017

Support @keyframes rules in user style sheets

Previously @keyframes rules in style sheets injected by extensions were
managed in ScopedStyleResolver along with other author rules. With
crrev.com/c/641294 we started treating these injected style sheets as
user style sheets and moved the management of such rules to StyleEngine.
This patch fixes a regression caused by this change whereby
StyleResolver is no longer able to find @keyframes rules from these
style sheets.

StyleEngine now maintains a map of animation names to their
corresponding @keyframes rules found in user style sheets. If
StyleResolver is unable to find a given @keyframes rule among its author
rules, it looks it up in StyleEngine next.

BUG= 632009 , 776661 

Change-Id: I90462a0721cc75da14ee041c1a70736e4df99fb8
Reviewed-on: https://chromium-review.googlesource.com/735263
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512411}
[modify] https://crrev.com/bed9cb9d56adac6f50313b0e620a1aa2f0d871ed/third_party/WebKit/Source/core/css/StyleEngine.cpp
[modify] https://crrev.com/bed9cb9d56adac6f50313b0e620a1aa2f0d871ed/third_party/WebKit/Source/core/css/StyleEngine.h
[modify] https://crrev.com/bed9cb9d56adac6f50313b0e620a1aa2f0d871ed/third_party/WebKit/Source/core/css/StyleEngineTest.cpp
[modify] https://crrev.com/bed9cb9d56adac6f50313b0e620a1aa2f0d871ed/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp

Comment 17 by rob@robwu.nl, Nov 1 2017

Status: Verified (was: Assigned)
I followed the STR from the report and can confirm that my PDF Viewer extension works as expected in 64.0.3256.0.

Sign in to add a comment