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

Issue 781011 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: 0
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

PNaCl just-in-time download is broken

Project Member Reported by waff...@chromium.org, Nov 2 2017

Issue description

The PNaCl JIT fetch is broken in the case where no component exists on disk (still works if an incompatible component exists on disk):

A call to CheckVersionCompatibility during the registration flow was removed by https://chromium-review.googlesource.com/c/chromium/src/+/627237 .
 
Labels: -Pri-3 M-61 Pri-2
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 3 2017

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

commit c59b0960eb1c82e9a3c18c97dc94bd2d464ca634
Author: Joshua Pawlicki <waffles@google.com>
Date: Fri Nov 03 00:09:07 2017

By default, claim PNaCl needs a JIT update: CheckVersionCompatibility
is not called in the registration flow unless there is a version of
PNaCl already on disk.

Bug:  781011 
Change-Id: I9be05564bd9a40df6dba7a8256cea286bf5aec24
Reviewed-on: https://chromium-review.googlesource.com/752049
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Commit-Queue: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/heads/master@{#513627}
[modify] https://crrev.com/c59b0960eb1c82e9a3c18c97dc94bd2d464ca634/chrome/browser/component_updater/pnacl_component_installer.cc

Comment 3 by laforge@google.com, Nov 3 2017

Labels: Merge-Request-63
Premptively merge-requesting to 63 (it's probably unrealistic to get into 62 given how late we are into the cycle).
Cc: abdulsyed@chromium.org
Labels: Merge-Request-62
Verified the fix in this morning's canary.

There's an M62 respin happening on Monday, maybe we want to catch that, too? +abdulsyed

Release owners:
The merge is low-risk (but also has limited impact: only new installs of Chrome are affected, and only for the first 6 minutes of the browser uptime). Do you prefer we take it or not? (One-line change that only touches the pnacl JIT install.)
What are the implications if we don't take this in M62? What is the full impact? My recommendation is to wait until M63. 
Users will not be able to use websites that use PNaCl within the first 6 minutes of browser use after they install Chrome.

I don't know how to quantify the number of sites that do this. This is certainly not a P0. (Presumably the number of broken sites is not too large because the bug went this long before being discovered.)

Example site: https://earth.google.com/web/
Cc: gov...@chromium.org
Labels: -Merge-Request-62 Merge-Rejected-62
Since impact is lower, recommendation is to target this for M63. 
Labels: -Merge-Request-63 Merge-Approved-63
Approving merge to M63 branch 3239 based on comment #4 and #7. Please merge ASAP. Thank you.
Labels: -Merge-Approved-63 merge-merged-3239
Merged @ https://chromium-review.googlesource.com/c/chromium/src/+/753804
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 3 2017

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

commit 90709468f3e0be750182fe0d5bb94c9a7eea4cd8
Author: Joshua Pawlicki <waffles@google.com>
Date: Fri Nov 03 17:23:45 2017

By default, claim PNaCl needs a JIT update: CheckVersionCompatibility
is not called in the registration flow unless there is a version of
PNaCl already on disk.

Bug:  781011 
Change-Id: I9be05564bd9a40df6dba7a8256cea286bf5aec24
Reviewed-on: https://chromium-review.googlesource.com/752049
Reviewed-by: Sorin Jianu <sorin@chromium.org>
Commit-Queue: Joshua Pawlicki <waffles@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#513627}(cherry picked from commit c59b0960eb1c82e9a3c18c97dc94bd2d464ca634)
Reviewed-on: https://chromium-review.googlesource.com/753804
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Cr-Commit-Position: refs/branch-heads/3239@{#366}
Cr-Branched-From: adb61db19020ed8ecee5e91b1a0ea4c924ae2988-refs/heads/master@{#508578}
[modify] https://crrev.com/90709468f3e0be750182fe0d5bb94c9a7eea4cd8/chrome/browser/component_updater/pnacl_component_installer.cc

Status: Fixed (was: Started)

Sign in to add a comment