Joel Maher ( :jmaher ) (UTC -8) Reporter
	Description • 11 years ago

found while testing locally.

	Ed Morley [:emorley] Assignee
	Comment 2 • 11 years ago

Comment on attachment 822359 [details] [diff] [review]
fix removal of directories to only do so if we find a crash (1.0)

In the java exception case, we still want to clean up the remote minidumps.

We should move the 'remoteminidumpdir' definition to outside of the 'if not found:', keep the removal of that directory outside of the conditional, but move the removal of 'minidumpdir' directory to inside the conditional.

	Ed Morley [:emorley] Assignee
	Comment 3 • 11 years ago

Thank you for spotting this btw :-)

	Ed Morley [:emorley] Assignee
	Comment 4 • 11 years ago

(Introduced by bug 910320)

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 5 • 11 years ago

valid point, round two

	Ed Morley [:emorley] Assignee
	Comment 6 • 11 years ago

Comment on attachment 822366 [details] [diff] [review]
move definitions of crash dirs outside of if found scope (2.0)

Review of attachment 822366 [details] [diff] [review]:
-----------------------------------------------------------------

We don't need to create the local minidump directory unless we're going to copy the minidumps over.

What I meant by comment 2 was to split the two parts (local vs remote directories) up and deal with one inside the conditional, and one outside :-)

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 7 • 11 years ago

so if we resolve the java exception there will be no remote or local minidump.  My first patch handled this as we would only create/remove the minidump directory if one was found.

	Ed Morley [:emorley] Assignee
	Comment 8 • 11 years ago

When there is a java exception, there is a minidump in the log, which is why we get the unhelpful stack traces.

I'll attach a patch :-)

	Ed Morley [:emorley] Assignee
	Comment 9 • 11 years ago

(This was my mess anyway :-))

	Ed Morley [:emorley] Assignee
	Comment 10 • 11 years ago

Look ok? :-)

	Ed Morley [:emorley] Assignee
	Comment 11 • 11 years ago

To be extra clear:

* If there is a java exception, there are still remote minidumps, which we presumably want to clean up.
* However, we only need to copy the remote minidumps locally if there wasn't a java exception found in the logcat.
* So cleanup of the local directory only needs to happen in the != java exception case.

	Ed Morley [:emorley] Assignee
	Comment 12 • 11 years ago

Side note:
Strange how we clear up the local directory in the Android case, but not desktop. Is this intentional?

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 13 • 11 years ago

Comment on attachment 822394 [details] [diff] [review]
Dont reference variables before they are defined

Review of attachment 822394 [details] [diff] [review]:
-----------------------------------------------------------------

ok, I totally missed the obvious.

	Joel Maher ( :jmaher ) (UTC -8) Reporter
	Comment 14 • 11 years ago

we really should clear up the local directory in the desktop case, that isn't intentional

	Ed Morley [:emorley] Assignee
	Comment 15 • 11 years ago

This code was fresh in my mind having done bug 910320 :-)

https://hg.mozilla.org/build/talos/rev/608228e6477f

(In reply to Joel Maher (:jmaher) from comment #14)
> we really should clear up the local directory in the desktop case, that
> isn't intentional

Filed bug 931071 :-)