Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(73)

Issue 463123004: Revert of r287982 Fix for race condition... (Closed)

Created:
6 years, 4 months ago by ivandavid
Modified:
6 years, 4 months ago
CC:
chromium-reviews, Dan Beam
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Revert of Fix for race condition where print preview hangs when attempting to print a PDF that hasn't loaded. (https://codereview.chromium.org/427723004/) Reason for revert: The changes fix an issue, but cause a more serious one. crbug.com/402402 Original issue's description: > Fix for race condition where print preview hangs when attempting to print a PDF that hasn't loaded. > > If |type| in RequestPrintPreview() equals PRINT_PREVIEW_USER_INITIATED_ENTIRE_FRAME and |is_loading_| is true, RequestPrintPreview() returns, > then is called again, right after |is_loading_| is set to false in DidStopLoading(). > > > BUG=376969 > TEST=See bug for steps to reproduce. > > Additional way to reproduce: > Build blink_tests and browser_tests in out/Release. Then in src/webkit/tools/layout_tests run this command: > ./run_webkit_tests.py --platform browser_test.linux source_pdf/ > > you can replace linux with win or mac depending on your platform. > > The program will attempt to save a PDF as a PDF through print preview. It will eventually fail because Print Preview will hang due to this bug. > If you want visual confirmation that this will happen open: > src/third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/browser_test_driver.py. > > Then add this line of code to the function cmd_line. > cmd.append('--enable-pixel-output-in-tests') > > Then run the script again. Enabling pixel output will also make it significantly more likely for the bug to occur. > Its also more likely to happen on windows for some reason. > > This is how I came across the bug. > > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287982 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289181

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -32 lines) Patch
M chrome/renderer/printing/print_web_view_helper.h View 2 chunks +0 lines, -7 lines 0 comments Download
M chrome/renderer/printing/print_web_view_helper.cc View 4 chunks +1 line, -25 lines 1 comment Download

Messages

Total messages: 11 (0 generated)
ivandavid
Created Revert of Fix for race condition where print preview hangs when attempting to print ...
6 years, 4 months ago (2014-08-12 18:51:03 UTC) #1
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-12 18:53:24 UTC) #2
Vitaly Buka (NO REVIEWS)
lgtm https://codereview.chromium.org/463123004/diff/1/chrome/renderer/printing/print_web_view_helper.cc File chrome/renderer/printing/print_web_view_helper.cc (left): https://codereview.chromium.org/463123004/diff/1/chrome/renderer/printing/print_web_view_helper.cc#oldcode1716 chrome/renderer/printing/print_web_view_helper.cc:1716: base::Bind(&PrintWebViewHelper::ShowScriptedPrintPreview, probably issue is some how related to ...
6 years, 4 months ago (2014-08-12 19:10:26 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ivandavid@chromium.org/463123004/1
6 years, 4 months ago (2014-08-12 19:12:44 UTC) #4
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium.win ...
6 years, 4 months ago (2014-08-12 22:39:44 UTC) #5
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 4 months ago (2014-08-13 00:11:53 UTC) #6
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/builds/6103)
6 years, 4 months ago (2014-08-13 00:11:56 UTC) #7
Vitaly Buka (NO REVIEWS)
The CQ bit was checked by vitalybuka@chromium.org
6 years, 4 months ago (2014-08-13 00:12:50 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ivandavid@chromium.org/463123004/1
6 years, 4 months ago (2014-08-13 00:18:21 UTC) #9
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel_swarming on tryserver.chromium.win ...
6 years, 4 months ago (2014-08-13 02:05:40 UTC) #10
commit-bot: I haz the power
6 years, 4 months ago (2014-08-13 03:23:30 UTC) #11
Message was sent while issue was closed.
Change committed as 289181

Powered by Google App Engine
This is Rietveld 408576698