Closed
Bug 1357791
Opened 4 years ago
Closed 3 years ago
Require mAppAssoc (vista+) API for extension/mimetype lookups from the OSHelperAppService
Categories
(Firefox :: File Handling, enhancement, P3)
Tracking
()
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: Gijs, Assigned: supersonic12910, Mentored)
References
Details
(Keywords: good-first-bug, Whiteboard: [lang=cpp][lang=c++])
Attachments
(1 file, 2 obsolete files)
Blocks like: https://dxr.mozilla.org/mozilla-central/rev/722fdbff1efc308a22060e75b603311d23541bb5/uriloader/exthandler/win/nsOSHelperAppService.cpp#125-138 and https://dxr.mozilla.org/mozilla-central/rev/722fdbff1efc308a22060e75b603311d23541bb5/uriloader/exthandler/win/nsOSHelperAppService.cpp#183-198 and https://dxr.mozilla.org/mozilla-central/rev/722fdbff1efc308a22060e75b603311d23541bb5/uriloader/exthandler/win/nsOSHelperAppService.cpp#441-458 either early return or have 'else' fallback paths. Especially in the former case, I believe we should at this point never reach the !mAppAssoc code, unless initialization failed due to OOM or similar, in which case other things are also likely to break. I think we should remove the fallback code and the nullcheck, potentially adding MOZ_(RELEASE_?)ASSERT for the mAppAssoc pointer being non-null. Jim, do you see any issues with this approach? Would the assert make sense?
Flags: needinfo?(jmathies)
![]() |
||
Comment 1•4 years ago
|
||
I see no reason to keep the old code around, you could also remove the Netscape 4.X registry stuff. Not sure if I'd do the release assert though, you'll end up with OOM failures here. I'd probably just fail the calls if the pointer is null.
Flags: needinfo?(jmathies)
Reporter | ||
Comment 2•4 years ago
|
||
Yep, the 4x stuff is bug 1357788.
Updated•4 years ago
|
Priority: -- → P3
Reporter | ||
Comment 3•3 years ago
|
||
To fix this bug, look through uriloader/exthandler/win/nsOSHelperAppService.cpp and for every `if (mAppAssoc)` block: - remove any comments that mention something like "Vista: use new thing" or otherwise imply there are other codepaths - replace the if () check with: NS_ENSURE_TRUE(mAppAssoc, NS_ERROR_NOT_AVAILABLE); - outdent the contents of the if block now that the if block is gone, removing any further braces or else blocks - remove any now-unreachable code (ie if the contents of the if block end with return NS_OK, anything after that in the same scope is now unreachable and should be removed)
I want to work on this bug.I am new to contributing on open source.Looks like a good place to start.
Flags: needinfo?(gijskruitbosch+bugs)
Reporter | ||
Comment 5•3 years ago
|
||
(In reply to Vasu from comment #4) > I want to work on this bug.I am new to contributing on open source.Looks > like a good place to start. Great! The first step would be getting Firefox built from source code so you can test patches and check they compile. There's a guide here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build . Otherwise, details for this bug are in comment 3. We can assign this bug to you when you put up your first version of the patch.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(vasusharma343)
Assignee | ||
Comment 6•3 years ago
|
||
(In reply to :Gijs from comment #5) > Great! The first step would be getting Firefox built from source code so you > can test patches and check they compile. There's a guide here: > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > Build_Instructions/Simple_Firefox_build . > > Otherwise, details for this bug are in comment 3. We can assign this bug to > you when you put up your first version of the patch. Hi there. Done there building, but I have a doubt though. https://dxr.mozilla.org/mozilla-central/rev/722fdbff1efc308a22060e75b603311d23541bb5/uriloader/exthandler/win/nsOSHelperAppService.cpp#442 I've just replaced if (msAppAssoc) with : NS_ENSURE_TRUE(msAppAssoc, nullptr); I'm wondering if that's correct or very wrong...
Assignee | ||
Comment 7•3 years ago
|
||
Reporter | ||
Updated•3 years ago
|
Attachment #8946107 -
Flags: review?(gijskruitbosch+bugs)
Reporter | ||
Comment 8•3 years ago
|
||
(In reply to Ashish Daulatabad from comment #6) > (In reply to :Gijs from comment #5) > > Great! The first step would be getting Firefox built from source code so you > > can test patches and check they compile. There's a guide here: > > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > > Build_Instructions/Simple_Firefox_build . > > > > Otherwise, details for this bug are in comment 3. We can assign this bug to > > you when you put up your first version of the patch. > > Hi there. Done there building, but I have a doubt though. > > https://dxr.mozilla.org/mozilla-central/rev/ > 722fdbff1efc308a22060e75b603311d23541bb5/uriloader/exthandler/win/ > nsOSHelperAppService.cpp#442 > > I've just replaced if (msAppAssoc) with : > > NS_ENSURE_TRUE(msAppAssoc, nullptr); > > I'm wondering if that's correct or very wrong... I think that makes sense in that context, yep. I'll have a proper look on Monday. Thanks for the patch!
(In reply to :Gijs from comment #8) > (In reply to Ashish Daulatabad from comment #6) > > (In reply to :Gijs from comment #5) > > > Great! The first step would be getting Firefox built from source code so you > > > can test patches and check they compile. There's a guide here: > > > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/ > > > Build_Instructions/Simple_Firefox_build . > > > > > > Otherwise, details for this bug are in comment 3. We can assign this bug to > > > you when you put up your first version of the patch. > > > > Hi there. Done there building, but I have a doubt though. > > > > https://dxr.mozilla.org/mozilla-central/rev/ > > 722fdbff1efc308a22060e75b603311d23541bb5/uriloader/exthandler/win/ > > nsOSHelperAppService.cpp#442 > > > > I've just replaced if (msAppAssoc) with : > > > > NS_ENSURE_TRUE(msAppAssoc, nullptr); > > > > I'm wondering if that's correct or very wrong... > > I think that makes sense in that context, yep. I'll have a proper look on > Monday. Thanks for the patch! If you change to NS_ENSURE_TRUE in the 3rd block then the return type of the method needs to be changed too but you can't change the return type of the method. So I am not sure what the workaround for that is.
Flags: needinfo?(vasusharma343)
Assignee | ||
Comment 10•3 years ago
|
||
(In reply to Vasu from comment #9) > If you change to NS_ENSURE_TRUE in the 3rd block then the return type of the > method needs to be changed too but you can't change the return type of the > method. So I am not sure what the workaround for that is. I don't think I've done right... Just looking at the code flow.
Reporter | ||
Comment 11•3 years ago
|
||
Comment on attachment 8946107 [details] [diff] [review] Bug 1357791 - Requirement of mAppAssoc (Vista+) API for extension lookup from nsOSHelperAppService.cpp Review of attachment 8946107 [details] [diff] [review]: ----------------------------------------------------------------- This is pretty good, but there's a few issues with the patch that should be fixed before it can land. You can make more changes and then use `hg commit --amend` to change the previous commit, and then export that commit to bugzilla again the way you did with this one. ::: uriloader/exthandler/win/nsOSHelperAppService.cpp @@ +80,5 @@ > *aHandlerExists = false; > if (aProtocolScheme && *aProtocolScheme) > { > // Vista: use new application association interface > + NS_ENSURE_TRUE(mAppAssoc, NS_ERROR_NOT_AVAILABLE); Nit: please use spaces to indent this instead of tabs. @@ +106,5 @@ > + nullptr, nullptr, nullptr, nullptr); > + *aHandlerExists = (err == ERROR_SUCCESS); > + // close the key > + ::RegCloseKey(hKey); > + } All of this (from `HKEY hKey;` to this closing `}` can be removed. @@ +146,5 @@ > + nsCOMPtr<nsIFile> app; > + nsAutoString appInfo(pResult); > + CoTaskMemFree(pResult); > + if (NS_SUCCEEDED(GetDefaultAppInfo(appInfo, _retval, getter_AddRefs(app)))) > + return NS_OK; Nit: please use spaces instead of tabs here too. @@ +372,1 @@ > } Nit: please remove the trailing (end-of-line) whitespace while we're here. @@ +375,2 @@ > } > + Nit: please remove the trailing (end-of-line) whitespace.
Attachment #8946107 -
Flags: review?(gijskruitbosch+bugs) → feedback+
Reporter | ||
Comment 12•3 years ago
|
||
(In reply to Ashish Daulatabad from comment #10) > (In reply to Vasu from comment #9) > > If you change to NS_ENSURE_TRUE in the 3rd block then the return type of the > > method needs to be changed too but you can't change the return type of the > > method. So I am not sure what the workaround for that is. > > I don't think I've done right... Just looking at the code flow. I think the third set of mAppAssoc changes in the patch (with NS_ENSURE_TRUE(mAppAssoc, nullptr)) looks OK. Let me know if you have more questions.
Assignee | ||
Comment 13•3 years ago
|
||
Assignee | ||
Comment 14•3 years ago
|
||
(In reply to :Gijs from comment #12) > I think the third set of mAppAssoc changes in the patch (with > NS_ENSURE_TRUE(mAppAssoc, nullptr)) looks OK. Let me know if you have more > questions. I've been facing some problems while modifying the blocks, so I've auto-indented everything. Would that be a problem while patching?
Reporter | ||
Comment 15•3 years ago
|
||
(In reply to Ashish Daulatabad from comment #14) > (In reply to :Gijs from comment #12) > > I think the third set of mAppAssoc changes in the patch (with > > NS_ENSURE_TRUE(mAppAssoc, nullptr)) looks OK. Let me know if you have more > > questions. > > I've been facing some problems while modifying the blocks, so I've > auto-indented everything. > > Would that be a problem while patching? Yes... it looks like the entire file is now indented with tabs? When fixing a bug, ideally you only touch the lines of code that you *need* to touch. We use spaces instead of tabs throughout the codebase, so it would be best to set up your editor such that it adapts to that being the case. So the patch looks OK at a glance, but we really need a version of it that keeps the same indenting, and applies cleanly to the current state of the tree (with spaces-for-indent instead of tabs-for-indent).
Assignee: nobody → supersonic12910
Status: NEW → ASSIGNED
Flags: needinfo?(supersonic12910)
Assignee | ||
Comment 16•3 years ago
|
||
(In reply to :Gijs from comment #15) > So the patch looks OK at a glance, but we really need a version of it that > keeps the same indenting, and applies cleanly to the current state of the > tree (with spaces-for-indent instead of tabs-for-indent). Would I need to indent the whole file with spaces?? That's tough... Still it's okay if I need to.
Reporter | ||
Comment 17•3 years ago
|
||
(In reply to Ashish Daulatabad from comment #16) > (In reply to :Gijs from comment #15) > > So the patch looks OK at a glance, but we really need a version of it that > > keeps the same indenting, and applies cleanly to the current state of the > > tree (with spaces-for-indent instead of tabs-for-indent). > > Would I need to indent the whole file with spaces?? > That's tough... > Still it's okay if I need to. Well, the file as it lives in version control is using spaces, I think? You can just use the version that's in mozilla-central, and when adapting the lines of code in your patch also use spaces.
Assignee | ||
Comment 18•3 years ago
|
||
(In reply to :Gijs from comment #17) > Well, the file as it lives in version control is using spaces, I think? You > can just use the version that's in mozilla-central, and when adapting the > lines of code in your patch also use spaces. Or I can do this too... Thanks...
Flags: needinfo?(supersonic12910)
Assignee | ||
Comment 19•3 years ago
|
||
(In reply to :Gijs from comment #17) > Well, the file as it lives in version control is using spaces, I think? You > can just use the version that's in mozilla-central, and when adapting the > lines of code in your patch also use spaces. Do I need to patch that again??
Reporter | ||
Comment 20•3 years ago
|
||
(In reply to Ashish Daulatabad from comment #19) > (In reply to :Gijs from comment #17) > > Well, the file as it lives in version control is using spaces, I think? You > > can just use the version that's in mozilla-central, and when adapting the > > lines of code in your patch also use spaces. > > Do I need to patch that again?? We'll need a patch that applies and has the spaces right, yes. You can probably import (`hg import https://bug1357791.bmoattachments.org/attachment.cgi?id=8946107` without the ` backticks) the first patch on top of vanilla mozilla-central, then modify the indent in the 1 or 2 places where it wasn't right, and apply the other changes in comment #11, and then commit --amend to create the next patch. Does that work? :-)
Flags: needinfo?(supersonic12910)
Assignee | ||
Comment 21•3 years ago
|
||
Attachment #8946107 -
Attachment is obsolete: true
Flags: needinfo?(supersonic12910)
Assignee | ||
Comment 22•3 years ago
|
||
(In reply to :Gijs from comment #20) > You can probably import (`hg import > https://bug1357791.bmoattachments.org/attachment.cgi?id=8946107` without the > ` backticks) the first patch on top of vanilla mozilla-central, then modify > the indent in the 1 or 2 places where it wasn't right, and apply the other > changes in comment #11, and then commit --amend to create the next patch. > > Does that work? :-) Yes, It did. Thanks
Reporter | ||
Comment 23•3 years ago
|
||
Comment on attachment 8947147 [details] [diff] [review] Bug 1357791 - Requirement of mAppAssoc (Vista+) API for extension lookup from nsOSHelperAppService.cpp Review of attachment 8947147 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: uriloader/exthandler/win/nsOSHelperAppService.cpp @@ +122,5 @@ > } > } > > + NS_ENSURE_TRUE(mAppAssoc, NS_ERROR_NOT_AVAILABLE); > + // Vista: use new application association interface Nit: we should remove the 'vista' comment here and below. I'll do that before landing this.
Attachment #8947147 -
Flags: review+
Reporter | ||
Updated•3 years ago
|
Attachment #8946692 -
Attachment is obsolete: true
Comment 24•3 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/21fea000a9fa require mAppAssoc (Vista+) API for extension lookup from nsOSHelperAppService.cpp, r=gijs
Comment 25•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/21fea000a9fa
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Updated•2 years ago
|
Depends on: CVE-2019-9801
You need to log in
before you can comment on or make changes to this bug.
Description
•