New issue
Advanced search Search tips

Issue 856161 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 25
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows
Pri: 1
Type: Bug
Proj-Servicification



Sign in to add a comment

network_service_browser_tests PDFTestFiles/PDFExtensionLoadTest.Load/4 fails pretty consistently in official builds

Project Member Reported by thakis@chromium.org, Jun 25 2018

Issue description

What steps will reproduce the problem?
1. Build with is_chrome_branded=true
2. Run PDFTestFiles/PDFExtensionLoadTest.Load/4 with network service enabled

What is the expected result?

Passes.

What happens instead of that?

Fails most of the time: https://ci.chromium.org/buildbot/chromium.clang/ToTWin64/?limit=200 (I enabled network_service_browser_tests in build 1646, that's why it's not failing before then)

https://logs.chromium.org/v/?s=chromium%2Fbb%2Fchromium.clang%2FToTWin64%2F1694%2F%2B%2Frecipes%2Fsteps%2Fnetwork_service_browser_tests%2F0%2Flogs%2FPDFTestFiles__x2f_PDFExtensionLoadTest.Load__x2f_4%2F0

../../chrome/browser/pdf/pdf_extension_test.cc(252): error: Expected equality of these values:
  PdfIsExpectedToLoad(pdf_file)
    Which is: false
  success
    Which is: true
pdf_private/cfuzz5.pdf
Google Test trace:
../../chrome/browser/pdf/pdf_extension_test.cc(248): pdf_private/cfuzz5.pdf


 
Cc: thestig@chromium.org dsinclair@chromium.org
 Issue 855221  has been merged into this issue.

Comment 2 by dougt@chromium.org, Jun 26 2018

Labels: Proj-Servicification-Canary Hotlist-KnownIssue
Status: Available (was: Untriaged)
This probably is a canary blocker if PDFs aren't loading properly.  If this is just a test problem, then we can remove the canary blocker.
Cc: -jam@chromium.org hnakashima@chromium.org
Owner: thestig@chromium.org
Status: Assigned (was: Available)
Lei, I found the list of PDFs being tested in /src/chrome/browser/pdf/pdf_extension_test.cc, but can't find the files themselves. Do you know where they are? cfuzz5.pdf is failing to load according to the logs.
Cc: jam@chromium.org

Comment 5 by thakis@chromium.org, Jun 26 2018

You need to sync src-internal to get these files. (See "1." in comment 0) That will map in, among other things, https://chrome-internal.googlesource.com/chrome/data/pdf_private/+/master
The problem is actually the other way around. cfuzz5.pdf is expected to fail to load, but is somehow considered successfully loaded.
With NetworkService enabled, the way the data arrives in DocumentLoaderImpl changes. As a result, DocumentLoader::Client::OnPendingRequestComplete() never gets called, and PDFiumEngine does not attempt to load the document until its fully downloaded. This causes it to succeed, whereas without NetworkService, attempting to load a partially downloaded document triggers a failure.
And that's because with NetworkService enabled, PepperPluginInstanceImpl::SendDidChangeView() calls from the browser actually makes it to the PDF plugin while a load is in progress. PDFiumEngine::CalculateVisiblePages() then calls DocumentLoaderImpl::ClearPendingRequests() and that affects loading.

Without NetworkService, the PDF plugin does not respond to SendDidChangeView() until after PDF loading finishes. I guess without NetworkService, the browser has to finish handling the network requests from the PDF plugin first?

Comment 9 by ajha@chromium.org, Jun 28 2018

Labels: M-69 officialtest OS-Linux
Fails on Linux 69.0.3475.0 on official.desktop builder also.
Cc: tsepez@chromium.org
 Issue 860757  has been merged into this issue.
Just to update:

Same issue seen on 69.0.3486.0 too on Linux64,Win Clang,Win 64 clang & Win Asan on official.desktop builder and Linux64 Trunk, Win trunk & Win64 trunk on continuous.desktop builder.

thestig@,Please take a look into it.
Thanks..!
Labels: -Pri-3 Pri-1
Owner: jam@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 18 by bugdroid1@chromium.org, Jul 25

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

commit b5706a3ad7605d299a1e2dbd777f72d9add7d009
Author: John Abd-El-Malek <jam@chromium.org>
Date: Wed Jul 25 00:13:40 2018

Fix crash when mimeHandlerPrivate.abortStream is called with the network service enabled.

This call isn't needed with the network service, since we pass the TransferrableURLLoader Mojo struct instead of stream URLs. When the PDF closes the main document load, the renderer will reset its Mojo pipe which will tell the network service to close the connection.

Bug: 705114,  856161 
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I1c87753f7820f6b1ce80bf06ace5aab899d1b903
Reviewed-on: https://chromium-review.googlesource.com/1148590
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577739}
[modify] https://crrev.com/b5706a3ad7605d299a1e2dbd777f72d9add7d009/chrome/browser/pdf/pdf_extension_test.cc
[modify] https://crrev.com/b5706a3ad7605d299a1e2dbd777f72d9add7d009/extensions/browser/guest_view/mime_handler_view/mime_handler_view_browsertest.cc
[modify] https://crrev.com/b5706a3ad7605d299a1e2dbd777f72d9add7d009/extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc
[modify] https://crrev.com/b5706a3ad7605d299a1e2dbd777f72d9add7d009/testing/buildbot/filters/mojo.fyi.network_browser_tests.filter

Status: Fixed (was: Started)
Ah, looks like my change made a pdf that used to crash before not crash (without the network service). I'll update the test expectations.
Project Member

Comment 22 by bugdroid1@chromium.org, Jul 27

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

commit a5767cd6614fbf504e156f3a44ba49da3aeb358e
Author: John Abd-El-Malek <jam@chromium.org>
Date: Fri Jul 27 05:15:41 2018

Update test expectations of PDFExtensionLoadTest.Load since one pdf doesn't crash anymore after r577739

Bug:  856161 
Change-Id: I68207ae97c8b0e863169991e541126f0854264cf
Reviewed-on: https://chromium-review.googlesource.com/1152532
Reviewed-by: Sam McNally <sammc@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578542}
[modify] https://crrev.com/a5767cd6614fbf504e156f3a44ba49da3aeb358e/chrome/browser/pdf/pdf_extension_test.cc

jam@: Shall we get the CL from C#22 merged to M-69 branch as well as we are still seeing PDFTestFiles/PDFExtensionLoadTest related test failures.

https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/linux64%20beta/builds/4746
https://uberchromegw.corp.google.com/i/official.desktop.continuous/builders/win64%20beta/builds/4366 
re: comment 23: PDFTestFiles/PDFExtensionLoadTest.Load is an entire suite of tests. Please file a separate bug for the PDFTestFiles/PDFExtensionLoadTest.Load/8 failure you are seeing and assign it to me.
Regarding the failing links above: these are beta branch and don't have the cl in comment 18 (which only fixes network_service_browser_tests). The failure is just in browser_tests path, which I didn't change, so it's orthogonal to this bug. I suspect these are flaky failures.

Sign in to add a comment