search

oalders / html-restrict

HTML::Restrict - Strip away unwanted HTML tags
Other
10 stars 9 forks source link

constructor returning undef #16

Closed ghost closed 11 years ago

ghost commented 11 years ago

I am getting similar failures to this test report: http://www.cpantesters.org/cpan/report/ad018ad4-b567-11e2-8c80-50d7c5c10595

Somehow this managed to install fine with cpanm when I upgraded to the latest version, but now it always dies with the error Can't call method "parse" on an undefined value. Reverting back to 2.1.4 fixes the problem.

I'm on OS X 10.7.5 with Perl 5.16.3.

oalders commented 11 years ago

I'm guessing this is related to the latest patch provided by @bleargh45

oalders commented 11 years ago

So, this is introduced by the change I mentioned earlier, but it's triggered by Moo 1.002000, which handles weak_ref differently. Running it under Moo 1.001000 is fine. I'll see what I can find out about this.

oalders commented 11 years ago

ilmari: oalders: that use of weak_ref is bogus. it's the $self you close over in the HTML::Parser callbacks that needs to be weakened [11:11am] oalders: ilmari: i will fix that [11:11am] oalders: thanks for your help [11:12am] ilmari: just add weaken($self) before return HTML::Parser->new(...) in _build_parser [11:13am] oalders: great. i'll try that today [11:14am] haarg: oalders: i mentioned on the ticket, but that was an explicit change to bring weak_ref+lazy in line with how Moose behaves. [11:15am] oalders: haarg: thanks. i wasn't aware of that [11:15am] haarg: i agree with ilmari that your usage looks wrong anyway. pretty sure parser would get re-calculated every time it was requested anyway. [11:16am] oalders: well, there is an accompanying test for that change and it did pass [11:16am] ilmari: haarg: no, it'd stay alive because of the closure [11:16am] oalders: https://github.com/oalders/html-restrict/blob/master/t/memory-leak.t [11:16am] ilmari: it's usually the attribute's reference to the owner that should be weakened [11:16am] oalders: is it not the correct test? [11:17am] haarg: ilmari: which closure? [11:17am] ilmari: oalders: that passes with moving the weakening from the attribute to the closure [11:17am] oalders: ok [11:17am] ilmari: haarg: the ones passed to HTML::Parser->new in _build_parser [11:18am] haarg: but that's a reference to $self from the parser. with the weak ref it wouldn't have any other references to the parser itself. [11:20am] ilmari: haarg: yeah, you're right [11:22am] haarg: ah, now i remember why it would have worked before. weak_ref+lazy was broken differently before. it would always recalculate. [11:22am] haarg: i had a fix for that (if you stored the return value somewhere) but we decided to just break (imo) weak_ref+lazy in the same way as moose. [11:24am] haarg: either way the old behavior was wrong and relying on it like HTML::Restrict did was invalid.

oalders commented 11 years ago

See also https://rt.cpan.org/Ticket/Display.html?id=85088

oalders commented 11 years ago

Fixed in 2.1.6