Require mAppAssoc (vista+) API for extension/mimetype lookups from the OSHelperAppService

RESOLVED FIXED in Firefox 60

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: Gijs, Assigned: supersonic12910, Mentored)

Tracking

({good-first-bug})

53 Branch
Firefox 60
good-first-bug
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: [lang=cpp][lang=c++])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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)
(Reporter)

Comment 2

2 years ago
Yep, the 4x stuff is bug 1357788.

Updated

2 years ago
Priority: -- → P3
(Reporter)

Comment 3

a year 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)
Mentor: gijskruitbosch+bugs
Keywords: good-first-bug
Whiteboard: [lang=cpp][lang=c++]

Comment 4

a year ago
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

a year 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

a year 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...
(Reporter)

Updated

a year ago
Attachment #8946107 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 8

a year 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!

Comment 9

a year ago
(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

a year 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

a year 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

a year 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 14

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year ago
Attachment #8946107 - Attachment is obsolete: true
Flags: needinfo?(supersonic12910)
(Assignee)

Comment 22

a year 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

a year 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

a year ago
Attachment #8946692 - Attachment is obsolete: true

Comment 24

a year 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
https://hg.mozilla.org/mozilla-central/rev/21fea000a9fa
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1527717
You need to log in before you can comment on or make changes to this bug.