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

Issue 133810 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
OOO
Closed: Jul 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 136567

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

ProgId registrations should respect MSDN rules.

Project Member Reported by gab@chromium.org, Jun 20 2012

Issue description

Version: 21.0.1180.0 dev-m
OS: Windows (all)

Suffixed registrations (progid(ChromeHTML.<suffix>, etc.) have always been done with the user's username.

This has always been wrong as the username can contain characters that are not allowed in a progid.

However, we only used to suffix registrations for the 2nd+ user-level install to be made default.

This had other problems and this is why we changed it to now always suffix user-level installs.

However, the fact we now always do it, makes the issue worst as it will have a wider impact.

Note: this only affects (and the fix to this also will) new installs as we prefer not to modify currently mis-registered chrome (i.e. we prefer bad registrations to UAC on update).

Note2: I created a user account that ends up creating a "technically" invalid progid on install, everything seems to work just fine with Windows (setting chrome default, etc.)... We still want to fix it however as consciously going against the MSDN standard is a very bad idea...


What we will do to fix this is use an encoded version of the SID which is unique for the user and could be mapped back to that user if necessary.
 

Comment 1 by kareng@google.com, Jun 22 2012

Labels: -ReleaseBlock-Beta ReleaseBlock-Stable
moving to stable blocker.
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 23 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=143782

------------------------------------------------------------------------
r143782 | gab@chromium.org | Fri Jun 22 19:01:39 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/shell_util_unittest.cc?r1=143782&r2=143781&pathrev=143782
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/shell_util.h?r1=143782&r2=143781&pathrev=143782
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/shell_util.cc?r1=143782&r2=143781&pathrev=143782

Use a better registration suffix that will always be unique while respecting the MSDN rules.

The suffix will now be the base32 encoding of the md5 hash of the user's username. This is always unique and can be determined many times deterministically without I/O access (as opposed to other methods we considered).

It respects all MSDN constraint for properties onto which it is appended.

This replaces the prior-style username suffixes for new installs.

Old-style suffixes (i.e. unsuffixed and username suffixed) will however be kept as is through future updates.

Design doc: https://docs.google.com/a/chromium.org/document/d/1qmcV3uYBh3JwvXhYkI7asg0nN7KfVMWVOzND4p0jQ3E/edit

BUG= 133810 
TEST= http://goo.gl/ZZ7gE
installer_util_unittests.exe --gtest_filter=ShellUtilTest.*

Review URL: https://chromiumcodereview.appspot.com/10617002
------------------------------------------------------------------------
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 24 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=143833

------------------------------------------------------------------------
r143833 | kbr@chromium.org | Sun Jun 24 06:14:12 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/shell_util_unittest.cc?r1=143833&r2=143832&pathrev=143833
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/shell_util.h?r1=143833&r2=143832&pathrev=143833
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/shell_util.cc?r1=143833&r2=143832&pathrev=143833

Revert 143782 - Use a better registration suffix that will always be unique while respecting the MSDN rules. (Triggered assertion failures in browser_tests blocking WebKit rolls.)

The suffix will now be the base32 encoding of the md5 hash of the user's username. This is always unique and can be determined many times deterministically without I/O access (as opposed to other methods we considered).

It respects all MSDN constraint for properties onto which it is appended.

This replaces the prior-style username suffixes for new installs.

Old-style suffixes (i.e. unsuffixed and username suffixed) will however be kept as is through future updates.

Design doc: https://docs.google.com/a/chromium.org/document/d/1qmcV3uYBh3JwvXhYkI7asg0nN7KfVMWVOzND4p0jQ3E/edit

BUG= 133810 
TEST= http://goo.gl/ZZ7gE
installer_util_unittests.exe --gtest_filter=ShellUtilTest.*

Review URL: https://chromiumcodereview.appspot.com/10617002

TBR=gab@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10667006
------------------------------------------------------------------------

Comment 4 by kbr@chromium.org, Jun 24 2012

Cc: kbr@chromium.org ukai@chromium.org
ukai@ triaged this better than me and discovered that this change seemed to work when browser_tests were are sharded, as on the main waterfall, but fails on bots where they aren't sharded.

Example failures:

http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=40088
http://build.chromium.org/p/chromium.webkit/builders/Win%20%28dbg%29/builds/6772

Comment 5 Deleted

On Over_install from 21.0.1180.0 --> Custom build from folder 21(21.0.1182.0) and Uninstalling chrome won't delete couple of entries which are listed below
HKEY_CURRENT_USER\Software\Classes\ChromeHTML.<Username>
HKEY_CLASSES_ROOT\ChromeHTML.<Username>

--Uninstall is not deleting the shortcut from "task bar" its the the same behavior on Win7/Win8.

Registrs which were not cleared :
-----------------------------------------
Win8 :
---------------------------------------
HKEY_CLASSES_ROOT\Chrome.admin 
HKEY_CURRENT_USER\Software\Classes\Chrome.admin.

Win7 :
-----------------------
HKEY_CLASSES_ROOT\ChromeHTML.<Username>
HKEY_CURRENT_USER\Software\Classes\ChromeHTML.<Username>

Comment 8 by gab@chromium.org, Jun 26 2012

I am aware of the un-deleted entries; however those can only occur if:

1) The installed Chrome was registered, but not made default (i.e. no UAC on Win7 to finish registration, but I'll have to investigate the Win8 .admin case).

2) The installed Chrome was installed from scratch with a build >= 21.0.1180.0.

2) Chrome then updates to my new build at which point (as it is not default/registered for shell_integration there is no risk upgrading to the new-style suffix) the base32 encoded suffix takes over (leaving the other one behind).

I originally didn't want to expose a new interface (i.e. expose GetOldUserSpecificRegistrySuffix) to clean this up as it only affected dev-channel. Given it might affect beta now maybe we want to clean this up anyways... thoughts?


Regarding the second issue with the shortcut not being removed, did you run Chrome at all after installing it and before uninstalling it (not that it would be correct to leave it in that case, but just looking for more details)?

Thanks,
Gab
--Yes, I launched chrome on fresh install and after over install with provided Min-installer navigated through couple of websites, When tried to uninstall it is not removing the task-bar shortcut.
--Attached the screenshot.
Win8shortcutnotdeleted.png
120 KB View Download

Comment 10 by gab@chromium.org, Jun 26 2012

I can't repro the shortcut issue.

When uninstalling by hand are you calling setup.exe --uninstall --multi-install --chrome (i.e. to match the arguments of the installer)?
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 6 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=145596

------------------------------------------------------------------------
r145596 | gab@chromium.org | Fri Jul 06 08:53:20 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/shell_util_unittest.cc?r1=145596&r2=145595&pathrev=145596
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/shell_util.h?r1=145596&r2=145595&pathrev=145596
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/setup/uninstall.cc?r1=145596&r2=145595&pathrev=145596
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/shell_util.cc?r1=145596&r2=145595&pathrev=145596

Use a better registration suffix that will always be unique while respecting the MSDN rules (2nd try). 

The suffix will now be the base32 encoding of the md5 hash of the user's username. This is always unique and can be determined many times deterministically without I/O access (as opposed to other methods we considered). 

It respects all MSDN constraints for properties onto which it is appended. 

This replaces the prior-style username suffixes for new installs. 

Old-style suffixes (i.e. unsuffixed and username suffixed) will however be kept as is through future updates. 

Design doc: https://docs.google.com/a/chromium.org/document/d/1qmcV3uYBh3JwvXhYkI7asg0nN7KfVMWVOzND4p0jQ3E/edit 

Note: This is the continuation of http://codereview.chromium.org/10617002/ which was reverted in https://chromiumcodereview.appspot.com/10667006/

BUG= 133810 ,  133173 
TEST= http://goo.gl/ZZ7gE 
installer_util_unittests.exe --gtest_filter=ShellUtilTest.*


Review URL: https://chromiumcodereview.appspot.com/10662052
------------------------------------------------------------------------

Comment 12 by gab@chromium.org, Jul 6 2012

Status: Fixed
I will verify it in the next dev-channel release and then we can merge it in beta.
Also awesome :). Keep us posted about how it looks in dev, Gab!

Comment 14 by gab@chromium.org, Jul 11 2012

Blockedon: chromium:136567
Labels: Merge-Requested
Status: Started
I tested it in the current dev channel and it's all great. The only nit is it exposed another bug ( issue 136567 ) which I'd like to merge the fix for in before this one.

Only the last commit needs to be merged (i.e. comment #11).

A roll of the internal deps is also necessary (as per https://chromereviews.googleplex.com/4647013/), but I'm not sure exactly how that works to merge src-internal changes into M21?
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 11 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=146217

------------------------------------------------------------------------
r146217 | gab@chromium.org | Wed Jul 11 14:57:48 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/installer/util/shell_util.cc?r1=146217&r2=146216&pathrev=146217

Convert SID string to ASCII before taking md5

(otherwise only half (the common half...:( ) of the sid string would get encoded...)

this change is super trivial and only fixes the resulting suffix

R=grt@chromium.org
BUG= 133810 
TEST=Make sure Chrome gets the proper suffix


Review URL: https://chromiumcodereview.appspot.com/10703147
------------------------------------------------------------------------

Comment 16 by kareng@google.com, Jul 11 2012

Labels: -Merge-Requested Merge-Approved
Project Member

Comment 17 by bugdroid1@chromium.org, Jul 12 2012

Labels: -Merge-Approved merge-merged-1180
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=146243

------------------------------------------------------------------------
r146243 | gab@google.com | Wed Jul 11 17:01:52 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/installer/setup/uninstall.cc?r1=146243&r2=146242&pathrev=146243
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/installer/util/shell_util.cc?r1=146243&r2=146242&pathrev=146243
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/installer/util/shell_util_unittest.cc?r1=146243&r2=146242&pathrev=146243
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/installer/util/shell_util.h?r1=146243&r2=146242&pathrev=146243

Merge 145596 - Use a better registration suffix that will always be unique while respecting the MSDN rules (2nd try). 

The suffix will now be the base32 encoding of the md5 hash of the user's SID. This is always unique and can be determined many times deterministically without I/O access (as opposed to other methods we considered). 

It respects all MSDN constraints for properties onto which it is appended. 

This replaces the prior-style username suffixes for new installs. 

Old-style suffixes (i.e. unsuffixed and username suffixed) will however be kept as is through future updates. 

Design doc: https://docs.google.com/a/chromium.org/document/d/1qmcV3uYBh3JwvXhYkI7asg0nN7KfVMWVOzND4p0jQ3E/edit 

Note: This is the continuation of http://codereview.chromium.org/10617002/ which was reverted in https://chromiumcodereview.appspot.com/10667006/

BUG= 133810 ,  133173 
TEST= http://goo.gl/ZZ7gE 
installer_util_unittests.exe --gtest_filter=ShellUtilTest.*


Review URL: https://chromiumcodereview.appspot.com/10662052

TBR=gab@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10703155
------------------------------------------------------------------------
Project Member

Comment 18 by bugdroid1@chromium.org, Jul 12 2012

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=146244

------------------------------------------------------------------------
r146244 | gab@google.com | Wed Jul 11 17:03:24 PDT 2012

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/1180/src/chrome/installer/util/shell_util.cc?r1=146244&r2=146243&pathrev=146244

Merge 146217 - Convert SID string to ASCII before taking md5

(otherwise only half (the common half...:( ) of the sid string would get encoded...)

this change is super trivial and only fixes the resulting suffix

R=grt@chromium.org
BUG= 133810 
TEST=Make sure Chrome gets the proper suffix


Review URL: https://chromiumcodereview.appspot.com/10703147

TBR=gab@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10703156
------------------------------------------------------------------------

Comment 19 by gab@chromium.org, Jul 12 2012

Cc: ananta@chromium.org cpu@chromium.org
Status: Fixed
For the record here is how the deps for delegate_execute were rolled to include my changes for M21 while excluding other changes made in M22 already:

Convert SID string to ASCII before taking md5 (copy of shell_util work) : https://chromereviews.googleplex.com/4617032/
Temporarily revert r26606: https://chromereviews.googleplex.com/4640027/
Revert r26500 temporarily: https://chromereviews.googleplex.com/4618034/
Roll DEPS to include my change and the reverts: https://chromereviews.googleplex.com/4616034/
Roll DEPS for delegate_execute on 1180 to incorporate suffix changes from  bug 133810 : https://chromereviews.googleplex.com/4636037/

And then to clean up (but at this point M21 merge was complete):
Unrevert 26500: https://chromereviews.googleplex.com/4620031/
Unrevert 26606: https://chromereviews.googleplex.com/4640028/
Roll DEPS to unrevert delegate_execute temporarily reverted changes: https://chromereviews.googleplex.com/4623030/

Cheers!
Gab
...phew. :) Thanks, Gab!
Project Member

Comment 21 by bugdroid1@chromium.org, Oct 13 2012

Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member

Comment 22 by bugdroid1@chromium.org, Nov 14 2012

The following revision refers to this bug:
    http://goto.ext.google.com/viewvc/chrome-internal?view=rev&revision=26556

------------------------------------------------------------------------
r26556 | gab@google.com | 2012-07-06T16:02:32.603973Z

------------------------------------------------------------------------
Project Member

Comment 23 by bugdroid1@chromium.org, Nov 14 2012

The following revision refers to this bug:
    http://goto.ext.google.com/viewvc/chrome-internal?view=rev&revision=26686

------------------------------------------------------------------------
r26686 | gab@google.com | 2012-07-12T00:04:48.745887Z

------------------------------------------------------------------------
Project Member

Comment 24 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Area-Internals -Internals-Install -Mstone-21 M-21 Cr-Internals Cr-Internals-Install
Project Member

Comment 25 by bugdroid1@chromium.org, Mar 14 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Project Member

Comment 26 by bugdroid1@chromium.org, Apr 5 2013

Labels: -Cr-Internals-Install Cr-Internals-Installer

Sign in to add a comment