380797 - email_in.pl dies with "Can't locate PatchReader.pm" when PatchReader not installed

Status: RESOLVED FIXED

(Bugzilla :: Incoming Email, defect)

Description

[Marc Branchaud]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: 

(This bug spawned from a thread on the support-bugzilla list.)

I upgraded from 3.0rc1 to 3.0 today, and decided to enable inbound 
emails.  I had a bit of trouble getting email_in.pl working.

I'm using fetchmail on the bugzilla machine to get emails from a POP 
server.  I can email comments to bugzilla, and the bug gets updated 
properly (I haven't tested anything other than comments yet).  However, 
a bounce email is also generated, with the following:

Can't locate PatchReader.pm in @INC (@INC contains: . /etc/perl 
/usr/local/lib/perl/5.8.8 /usr/local/share/perl/5.8.8 /usr/lib/perl5 
/usr/share/perl5 /usr/lib/perl/5.8 /usr/share/perl/5.8 
/usr/local/lib/site_perl)

I fixed this by doing
	ln -s Bugzilla/Attachment/PatchReader.pm .
in my Bugzilla directory.



Reproducible: Always

Steps to Reproduce:
1. Configure fetchmail to invoke email_in.pl via fetchmail's "mda" option.
2. Send an email to bugzilla (i.e. to the account that fetchmail polls for mail to pipe through email_in.pl).  In my testing I reply to a Bugzilla email and simply add a comment to the bug.
3. Run fetchmail to fetch the mail.
Actual Results:  
Bugzilla updates the bug fine, but a bounce email is generated with the error:

Can't locate PatchReader.pm in @INC (@INC contains: . /etc/perl 
/usr/local/lib/perl/5.8.8 /usr/local/share/perl/5.8.8 /usr/lib/perl5 
/usr/share/perl5 /usr/lib/perl/5.8 /usr/share/perl/5.8 
/usr/local/lib/site_perl)


Expected Results:  
No bounce email should be generated.

After some back-and-forth with Max Kanat-Alexander (see the support-bugzilla thread), we figure that the problem seems to be arising somewhere inside process_bug.cgi.

I do not have the PatchReader CPAN module installed on my system.

Comment 1

[Max Kanat-Alexander]

It'd be nice if somebody internal could attempt to reproduce this.

Comment 2

[Max Kanat-Alexander]

Also, if you could run email_in.pl with -vvv and attach the results, that would help, too.

Comment 3

[Marc Branchaud]

Attachment: email_in.pl run with -vvv

Output when running fetchmail configured to run email_in.pl with -vvv

Comment 4

[Max Kanat-Alexander]

Weird. That looks totally normal to me. Maybe I just never tested on a system that doesn't have PatchReader, and there's some problem when you don't have it installed. I could certainly check.

Comment 5

[Max Kanat-Alexander]

Okay, I can reproduce this. All you have to do is uninstall PatchReader, and it happens. It really shouldn't, but it does. I'll look into it.

Comment 6

[Max Kanat-Alexander]

Attachment: v1

Okay, this fixes the bug. Let me explain:

Due to a bug in Perl, $SIG{__DIE__} handlers are called even inside of an eval {}.  So the eval'ed failure to require PatchReader was calling our handler, and our handler was acting as though the script had died.

We can call *normal* "die" inside of the handler to handle that case. So that's what I'm doing here.

Comment 7

[Marc Branchaud]

I've confirmed that this patch fixes the problem for me.

Comment 8

[Frédéric Buclin]

Comment on attachment 264945 [details] [diff] [review]
v1

I cannot test this patch as I have some missing packages required by the script.

Comment 9

[Vlad Dascalu]

Comment on attachment 264945 [details] [diff] [review]
v1

This looks like a sound explanation. Marc tested the patch and confirms that it fixes the issue for him.

My code review matches Max's explanation of it. http://www.perlmonks.org/index.pl?node_id=403#item__S for those wanting to look up the internals.

Comment 10

[Frédéric Buclin]

Comment on attachment 264945 [details] [diff] [review]
v1

>+    die(@_) if (defined $^S && $^S == 1);

Now thinking about what I did in Bugzilla::Error, why not writing |if $^S| only?

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> Now thinking about what I did in Bugzilla::Error, why not writing |if $^S|
> only?

  See: http://perldoc.perl.org/perlvar.html#$^S

Comment 12

[Max Kanat-Alexander]

LpSolit: I'm OK with checking this in now if you're OK with it, as far as QA goes. Do you guys even do QA on email_in?

Comment 13

[Frédéric Buclin]

No problem. Check it in. manu and achild did some tests on it, but this looks safe.

Comment 14

[Frédéric Buclin]

(In reply to comment #11)
>   See: http://perldoc.perl.org/perlvar.html#$^S

This URL says I'm right as you are only interested when the value is true. If it's either undefined or false, you don't care.

Comment 15

[Max Kanat-Alexander]

Oh yeah, I suppose you're right. Anyhow, we can leave it like this for now, or I can fix it on checkin if you want.

Comment 16

[Frédéric Buclin]

(In reply to comment #15)
> Oh yeah, I suppose you're right. Anyhow, we can leave it like this for now, or
> I can fix it on checkin if you want.

Do what you want. :)

Comment 17

[Max Kanat-Alexander]

I used LpSolit's way, because it makes more sense.

tip:

Checking in email_in.pl;
/cvsroot/mozilla/webtools/bugzilla/email_in.pl,v  <--  email_in.pl
new revision: 1.6; previous revision: 1.5
done

3.0:

Checking in email_in.pl;
/cvsroot/mozilla/webtools/bugzilla/email_in.pl,v  <--  email_in.pl
new revision: 1.5.2.1; previous revision: 1.5
done

Comment 18

[Teemu Mannermaa (:wicked)]

> >   See: http://perldoc.perl.org/perlvar.html#$^S
> This URL says I'm right as you are only interested when the value is true. If
> it's either undefined or false, you don't care.

URL says undefined can happen when parsing eval and are inside DIE handler. Doesn't this mean undefined is also what we are interested in as this code is part of such handler?

> manu and achild did some tests on it, but this looks safe.

I did also test email_in.pl briefly on 3.0 branch when I tried to test this patch on trunk. Trunk version was broken (which prevented my review) but 3.0 branch version seemed to work then (without this patch).

Comment 19

[Max Kanat-Alexander]

(In reply to comment #18)
> URL says undefined can happen when parsing eval and are inside DIE handler.
> Doesn't this mean undefined is also what we are interested in as this code is
> part of such handler?

  No, not really. By "parsing eval", I'm pretty sure they mean "compiling the code inside of the eval". So we still want to use die_handler on that case.