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)

53 Branch
enhancement

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)
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)
Yep, the 4x stuff is bug 1357788.
Priority: -- → P3
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)
Mentor: gijskruitbosch+bugs
Keywords: good-first-bug
Whiteboard: [lang=cpp][lang=c++]
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)
(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)
(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...
Attachment #8946107 - Flags: review?(gijskruitbosch+bugs)
(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)
(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.
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+
(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.
(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?
(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)
(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.
(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.
(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)
(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??
(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)
Attachment #8946107 - Attachment is obsolete: true
Flags: needinfo?(supersonic12910)
(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
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+
Attachment #8946692 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/21fea000a9fa
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: CVE-2019-9801
You need to log in before you can comment on or make changes to this bug.