MailImpl.setMessage and possible NPE: regression from 2.2.0 and 2.3.0rc1

• Key: JAMES-609
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Stefano Bagnara
• Reporter: Stefano Bagnara
• Component/s: James Core
• Affects Version/s: 2.3.0
• Fix Version/s: 2.3.0

Description

The change introduced in rc2 to fix the file leaks has produced an NPE in a custom mailet.

I suggest to revert the setMessage changes removing the dispose() call for the old message and to put the dispose code in the bundled calling mailets.

This is not a good fix in the long term, but for 2.3.0 I would prefer to keep compatibility with 2.2.0 and to have a leak instead of NPE (difficult to understand by most users).

Comments

[Stefano Bagnara]

I believe the NPE could be thrown when saving the message after having called SMIMECheckSignature when "stripSignature && strippedMessage != null" because it calls setMessage with the same message.

[Stefano Bagnara]

This is a possible fix to the problem.
Few changes are on the tests to solve some temporary file not release (24 before the fix, 20 after the fix).

Now that I wrote it I'm not sure about this in 2.3: they are a lot of changes... maybe it is better to add a check in setMessage to see if the new message is the same of the old one...

I have to think more about this...

[Noel J. Bergman]

I agree that it would be better to check in setMessage if this.message == message.

None of these is a perfect solution, since the explicit dispose call reintroduces the problems that exist in other languages with explicit deallocation: how to know when it is safe to deallocate (and clean-up). What if someone were to call getMessage, put the old message object into a new Mail, and put a new message in the old Mail?

But the least objectionable change for now is probably the check in setMessage. The logic for that one is clear: if we are setting the message to the one we already have, do nothing.

[Stefano Bagnara]

We agreed to fix this with the small "identity check" fix.
Applied to both 2.3 branch and trunk.

[Danny Angus]

Closing issue fixed in released version.