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

Status: RESOLVED FIXED

(Firefox :: File Handling, enhancement, P3)

Description

[:Gijs (he/him)]

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?

Comment 1

[Jim Mathies [:jimm]]

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.

Comment 2

[:Gijs (he/him)]

Yep, the 4x stuff is bug 1357788.

Comment 3

[:Gijs (he/him)]

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)

Comment 4

[Vasu]

I want to work on this bug.I am new to contributing on open source.Looks like a good place to start.

Comment 5

[:Gijs (he/him)]

(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.

Comment 6

[Ashish Daulatabad]

(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...

Comment 7

[Ashish Daulatabad]

Attachment: Bug 1357791 - Requirement of mAppAssoc (Vista+) API for extension lookup from nsOSHelperAppService.cpp

Comment 8

[:Gijs (he/him)]

(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

[Vasu]

(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.

Comment 10

[Ashish Daulatabad]

(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 11

[:Gijs (he/him)]

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.

Comment 12

[:Gijs (he/him)]

(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.

Comment 13

[Ashish Daulatabad]

Attachment: Bug 1357791 - Requirement of mAppAssoc (Vista+) API for extension lookup from nsOSHelperAppService.cpp

Comment 14

[Ashish Daulatabad]

(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?

Comment 15

[:Gijs (he/him)]

(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).

Comment 16

[Ashish Daulatabad]

(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.

Comment 17

[:Gijs (he/him)]

(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.

Comment 18

[Ashish Daulatabad]

(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...

Comment 19

[Ashish Daulatabad]

(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??

Comment 20

[:Gijs (he/him)]

(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? :-)

Comment 21

[Ashish Daulatabad]

Attachment: Bug 1357791 - Requirement of mAppAssoc (Vista+) API for extension lookup from nsOSHelperAppService.cpp

Comment 22

[Ashish Daulatabad]

(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 23

[:Gijs (he/him)]

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.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21fea000a9fa
require mAppAssoc (Vista+) API for extension lookup from nsOSHelperAppService.cpp, r=gijs

Comment 25

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/21fea000a9fa