Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix NPE on MethodFormat.getCodeSource #751

Closed
wants to merge 1 commit into from
Closed

fix NPE on MethodFormat.getCodeSource #751

wants to merge 1 commit into from

Conversation

bySabi
Copy link

@bySabi bySabi commented Jul 17, 2014

When use a custom classloader protectionDomain.getCodeSource().getLocation() can return null.

@bySabi
Copy link
Author

bySabi commented Jul 18, 2014

With some custom classloader protectionDomain.getCodeSource()can be null too.
is safe to define?

 private String getCodeSource(Method method) {
   return method.getDeclaringClass().getName();
}

@paoloambrosio
Copy link
Member

On what platform is this NPE happening?

@bySabi
Copy link
Author

bySabi commented Aug 2, 2014

Well I was testing Cucumber-jvm with Robolectric classloader AsmInstrumentingClassLoader, see my post: https://groups.google.com/forum/#!topic/cukes/Eq4AG4bMo9E

The problem with protectionDomain.getCodeSource() relate to custom classloader who not extend SecureClassLoader class see link: http://docstore.mik.ua/orelly/java-ent/security/ch03_04.htm
Then if possible get NPE with other classloader.

Why not just use? :

 private String getCodeSource(Method method) {
   return method.getDeclaringClass().getName();
}

@aslakhellesoy
Copy link
Contributor

@bySabi the old code gives the path to the source file. Your PR looks good. It's a good fallback for when the source is unavailable.

@brasmusson
Copy link
Contributor

If I read the comments correctly, getProtectionDomain(), getProtectionDomain().getSourceCode() or getProtectionDomain().getSourceCode().getLocation() may return null (on some platforms/class loaders).
Given that (and that the PR currently only handles the first and third case) I think it would be better to use a try - catch construct to handle all these cases:

  try {
    ProtectionDomain protectionDomain = method.getDeclaringClass().getProtectionDomain();
    return protectionDomain.getCodeSource().getLocation().toExternalForm();
  } catch (Exception e) {
    // getProtectionDomain() returns null on some platforms (for example on Android)
    return method.getDeclaringClass().getName();
  }

(the fallback return method.getDeclaringClass().getName(); is maybe not wrong, but kind of redundant since the full message in that case would be:

<qualified class name>.<method name> in <qualified class name>

where the second <qualified class name> comes from the getSourceCode() method)

@bySabi
Copy link
Author

bySabi commented Aug 3, 2014

Your are right @brasmusson try-catch is a better aproach.

@bySabi
Copy link
Author

bySabi commented Aug 4, 2014

I create a new PR: #757 when suggested @brasmusson try-catch code.

@brasmusson brasmusson closed this in 151f210 Aug 5, 2014
@lock
Copy link

lock bot commented Oct 25, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants