Closed
Bug 118216
Opened 23 years ago
Closed 22 years ago
Can crash in engine installing to folder without sufficient permissions
Categories
(Core Graveyard :: Installer: XPInstall Engine, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.0
People
(Reporter: slogan, Assigned: dprice)
Details
(Whiteboard: [adt2] [ETA 4-23])
Attachments
(1 file, 3 obsolete files)
1.56 KB,
patch
|
slogan
:
review+
dveditz
:
superreview+
scc
:
approval+
|
Details | Diff | Splinter Review |
A patch on a separate bug http://bugzilla.mozilla.org/show_bug.cgi?id=89436 keeps the Linux/Unix stub and blob installers from running into this, but we still need to address it in the backend in case other applications using XPI don't account for it.
Updated•23 years ago
|
Status: NEW → ASSIGNED
Updated•22 years ago
|
Target Milestone: mozilla0.9.9 → mozilla1.0
Attachment #63552 -
Flags: review+
--> dprice to get sr=, and adt approval for this crasher
Assignee: dveditz → dprice
Status: ASSIGNED → NEW
Assignee | ||
Comment 4•22 years ago
|
||
there was some drifting in the code. This patch is a unified diff that's up to date.
Attachment #63552 -
Attachment is obsolete: true
Comment 5•22 years ago
|
||
Please get a module owner level review then ping me again for a sr. Thanks!
Comment 6•22 years ago
|
||
Comment on attachment 78866 [details] [diff] [review] updated patch instead of !NS_SUCCEEDED(rv), I think you should use NS_FAILED(rv)
Assignee | ||
Comment 7•22 years ago
|
||
Changed to using NS_FAILED()
Attachment #78866 -
Attachment is obsolete: true
Comment 8•22 years ago
|
||
Comment on attachment 79389 [details] [diff] [review] patch 2 >- if (bTryProfileDir) >+ if (bTryProfileDir && !nsSoftwareUpdate::GetProgramDirectory()) The !nsSoftwareUpdate::GetProgramDirectory() is confusing and needs commenting that it basically means "not in the install wizard". >- if (!dirSvc) return NS_ERROR_FAILURE; >+ if (!dirSvc || NS_FAILED(rv)) return NS_ERROR_FAILURE; > dirSvc->Get(NS_APP_USER_PROFILE_50_DIR, NS_GET_IID(nsIFile), > getter_AddRefs(iFile)); Seems strange to me that we'd check the return value of do_GetService() in addition to dirSvc not being null (which many argue is a redundant check) and then not check the status of the Get() call which *might* fail if some embedding app didn't define that target. > if (!nsSoftwareUpdate::GetLogName()) > rv = iFile->Append(INSTALL_LOG); > else > rv = iFile->Append(nsSoftwareUpdate::GetLogName()); and if it does fail, then here we go boom.
Attachment #79389 -
Flags: needs-work+
Assignee | ||
Comment 9•22 years ago
|
||
Adding additional checks
Attachment #79389 -
Attachment is obsolete: true
Reporter | ||
Comment 10•22 years ago
|
||
Comment on attachment 79710 [details] [diff] [review] patch 3 r=syd
Attachment #79710 -
Flags: review+
Comment 11•22 years ago
|
||
Comment on attachment 79710 [details] [diff] [review] patch 3 >- if (bTryProfileDir) >- { >+ if (bTryProfileDir && !nsSoftwareUpdate::GetProgramDirectory()) >+ { >+ // Not in the stub installer > // failed to create the log file in the application directory > // so try to create the log file in the user's profile directory That's not that great a comment, it won't be clear to anyone that it has anything to do with the cryptic !nsSoftwareUpdate::blah(). Short of adding an in-line nsSoftwareUpdate::InWizard() for readability that evaluates to the above (since we use this idiom everywhere we should do that eventually) your added comment would make more sense *after* the existing comments: // but only if we're not in the stub installer sr=dveditz if you make the comment change.
Attachment #79710 -
Flags: superreview+
Comment 12•22 years ago
|
||
Pls land this on the trunk today. Once Jimmy has verified it on the trunk, we will consider for inclusion on the 1.0 branch.
Whiteboard: [adt3] → [adt3] [ETA Needed]
Reporter | ||
Comment 13•22 years ago
|
||
Once landed in the trunk, please mark "FIXED" so that testing on the trunk is commenced, and track this bug for verified, nominate adt1.0.0 once you get the verified.
Assignee | ||
Updated•22 years ago
|
Whiteboard: [adt3] [ETA Needed] → [adt3] [ETA 4-23]
Assignee | ||
Comment 14•22 years ago
|
||
landed on the trunk friday, marking fixed
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 15•22 years ago
|
||
Build: 2002-04-22-10-trunk(LINUX) Engine does not crash when installing to folder without sufficient permissions. As expeected, file(s)/folder(s) are not created in folder without sufficient permissions. As expected, Install.log is created in .mozilla directory. Error -202 (ACCESS_DENIED) is reported from Install.log which appears to be accurate. Marking Verified for TRUNK!
Status: RESOLVED → VERIFIED
Comment 16•22 years ago
|
||
adding adt1.0.0+. Please check this into the branch as soon as possible and add the fixed1.0.0 keyword after getting drivers approval.
Comment 17•22 years ago
|
||
Comment on attachment 79710 [details] [diff] [review] patch 3 a=scc for checkin to the mozilla 1.0 branch
Attachment #79710 -
Flags: approval+
Assignee | ||
Updated•22 years ago
|
Keywords: fixed1.0.0
Comment 18•22 years ago
|
||
Build: 2002-04-25-08-1.0.0(LINUX) Looks good on BRANCH as well. Adding keyword, verified1.0.0.
Keywords: fixed1.0.0 → verified1.0.0
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•