NativeDeleteKey needs to deal with non-existent keys gracefully

VERIFIED FIXED in mozilla1.0.1

Status

Core Graveyard
Installer: XPInstall Engine
P1
blocker
VERIFIED FIXED
15 years ago
2 years ago

People

(Reporter: Syd Logan, Assigned: Syd Logan)

Tracking

Trunk
mozilla1.0.1
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt1 rtm] [ETA 08/20])

Attachments

(1 attachment, 1 obsolete attachment)

951 bytes, patch
Joe Hewitt (gone)
: review+
dveditz
: superreview+
Judson Valeski
: approval+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
NativeDeleteKey can be called after the key has been deleted, which will lead to 
an error. For example, installer script schedules same key twice for deletion. 
The first will succeed during performInstall, the second will fail. It should be 
more tolerant of this (but debug runtime should flag this error with an assert).

Patch on its way.
(Assignee)

Updated

15 years ago
Severity: critical → blocker
Keywords: nsbeta1+
Whiteboard: [adt1 rtm]
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 1

15 years ago
Created attachment 95836 [details] [diff] [review]
patch to fix this
(Assignee)

Updated

15 years ago
Attachment #95836 - Attachment is obsolete: true
(Assignee)

Comment 2

15 years ago
Created attachment 95839 [details] [diff] [review]
Patch with debug asserts added

Forgot to add a debug assert, plus some nitpicking about how to code
conditional statements.
Comment on attachment 95839 [details] [diff] [review]
Patch with debug asserts added

sr=dveditz  (explicitly testing against PR_TRUE doesn't match the style of the
file, though)
Attachment #95839 - Flags: superreview+

Comment 4

15 years ago
Comment on attachment 95839 [details] [diff] [review]
Patch with debug asserts added

r=hewitt
Attachment #95839 - Flags: review+
(Assignee)

Updated

15 years ago
Keywords: adt1.0.1
(Assignee)

Comment 5

15 years ago
fix landed in trunk, should be in the trunk 8/19/2002 builds.
Status: NEW → ASSIGNED

Comment 6

15 years ago
Marking as Resolved/Fixed per comment Comment #5 From syd@netscape.com.

paw: who can verify this fix on the trunk for us?
Blocks: 143047
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Keywords: mozilla1.0.1
Priority: -- → P1
Resolution: --- → FIXED
Whiteboard: [adt1 rtm] → [adt1 rtm] [ETA 08/20]
Target Milestone: mozilla1.0 → mozilla1.0.1

Comment 7

15 years ago
Jimmy, who is the QA contact

Comment 8

15 years ago
adding adt1.0.1+
Keywords: adt1.0.1 → adt1.0.1+

Comment 9

15 years ago
Build: 2002-08-19-08-trunk(WIN)

This is not working for me.  Marking reopen.  The install.log returns:
-------------------------------------------------------------------------------
http://jimbob/jars/a_winreg_deletekey.xpi  --  08/19/2002 15:38:13
-------------------------------------------------------------------------------

     Acceptance: a_winreg_deletekey (version 1.2.3.4)
     ------------------------------

     ** ERROR (-242): Delete Registry Key: HKEY_LOCAL_MACHINE\Software\WinReg
Test Key\subkey1\subkey2 []

     Install cancelled by script  --  08/19/2002 15:38:13

I am expecting something like "Trying to delete a key that doesn't exist
anymore. Possible causes include calling deleteKey for the same key multiple times."

The test I am running is http://jimbob/jars/a_winreg_deletekey.xpi

Install script:
initInstall("Acceptance: a_winreg_deletekey", "a_winreg_deletekey", "1.2.3.4", 0); 

var winreg = getWinRegistry();

winreg.setRootKey(winreg.HKEY_LOCAL_MACHINE);
winreg.deleteKey("Software\\WinReg Test Key\\subkey1\\subkey2");

if (0 == getLastError())
	performInstall();
else
	cancelInstall();
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 10

15 years ago
I should add that -242 does sound correct though.

KEY_DOES_NOT_EXIST          = -242

Updated

15 years ago
Attachment #95839 - Flags: approval+

Comment 11

15 years ago
 please checkin to the MOZILLA_1_0_BRANCH branch. once there, remove the
"mozilla1.0.1+" keyword and add the "fixed1.0.1" keyword. 
Keywords: mozilla1.0.1 → mozilla1.0.1+
(Assignee)

Comment 12

15 years ago
that is a different error, we are seeing an error '2' in the native installs.
The -242 you are seeing is probably valid and is due to a check in the prep
stage. The 2 error is something totally different. Please file another bug if
you think this is wrong behavior to see -242 (seems like right behavior to me),
we are trying to resolve a specific bug in the native stub installer.
Status: REOPENED → RESOLVED
Last Resolved: 15 years ago15 years ago
Keywords: fixed1.0.1
Resolution: --- → FIXED

Comment 13

15 years ago
-242 is correct to me.  However, I can use some guidance to identify where this
Error "2" appears from the native installer.  Thanks.
(Assignee)

Comment 14

15 years ago
Well, write a script that tries to delete the same key twice. I'm not sure how
far up the chain the -2 will be propogated in your test scaffolding, but in the
stub installer, it will generate a '2' error. Perhaps modifying an install.js
executed in one of the stub or blob install xpis will do the trick.

Comment 15

15 years ago
jimmylee: can you pls verify this as fixed on the trunk and 1.0 branch, then
replace the "fixed1.0.1" keyword with "verified1.0.1"?
Keywords: mozilla1.0.1+

Comment 16

15 years ago
Build: 2002-08-21-09-trunk(WIN), 2002-08-21-08-1.0(WIN)

Looks good on both TRUNK and BRANCH.  Thanks to Gary for providing the machine
that reliably duplicates the problem.

Marking Verified.  Adding keyword "verified1.0.1".
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.1 → verified1.0.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.