When viewing "more stats" link on a number component in a webform with a single submission, I get the following out of memory error message due to an infinite loop:
Fatal error: Allowed memory size of 201326592 bytes exhausted (tried to allocate 20 bytes) in webform/components/number.inc on line 444

CommentFileSizeAuthor
#9 webform-fix_infinite_loop-1804858-9.patch513 bytespdrake
#7 1804858-7.patch512 bytesbxtaylor
#4 1804858-4.patch436 bytesbxtaylor
#2 Screen Shot 2012-10-08 at 5.49.04 PM.png39.16 KBbxtaylor
#1 webform-fix_infinite_loop-1804858-1.patch5.06 KBpdrake
#1 webform-fix_infinite_loop_no_whitespace-1804858-1.patch689 bytespdrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pdrake’s picture

pdrake CreditAttribution: pdrake commented
Status: Active » Needs review
FileSize
webform-fix_infinite_loop_no_whitespace-1804858-1.patch689 bytes
webform-fix_infinite_loop-1804858-1.patch5.06 KB

This patch excludes the standard deviation table if the standard deviation is 0, which solves the infinite loop that causes the memory limit error. The second patch ("no_whitespace") was created by running git diff -w and is not intended to be used, but makes review much easier.

bxtaylor’s picture

bxtaylor CreditAttribution: bxtaylor commented
Status: Needs review » Reviewed & tested by the community
FileSize
Screen Shot 2012-10-08 at 5.49.04 PM.png39.16 KB

I was able to reproduce the bug when only one submission exists. The patch applied cleanly and fixed the memory exhaustion issue.

From what I see, this prevents the distribution table from being printed if there's only one submission.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Thanks guys. If we're skipping the entire set of processing, could we short-circuit the function call and just use:

if (empty($stddev)) {
  return $rows;
}

I don't normally advocate multiple returns, but in this situation it avoids a lot of indentation and makes the code clearer. Acceptable?

bxtaylor’s picture

bxtaylor CreditAttribution: bxtaylor commented
Status: Reviewed & tested by the community » Needs review
FileSize
1804858-4.patch436 bytes

Works for me. Here's a reroll...

pdrake’s picture

pdrake CreditAttribution: pdrake commented
Status: Needs review » Reviewed & tested by the community

I'm good with just short-circuiting here. It may be worthy of a comment to ensure nobody adds additional analysis rows after the stddev table rows?

pdrake’s picture

pdrake CreditAttribution: pdrake commented
Status: Reviewed & tested by the community » Needs work

Conditional needs proper spacing.

bxtaylor’s picture

bxtaylor CreditAttribution: bxtaylor commented
Status: Needs work » Needs review
FileSize
1804858-7.patch512 bytes

Rerolled with conditional spacing fix and comment.

pdrake’s picture

pdrake CreditAttribution: pdrake commented
Status: Needs review » Reviewed & tested by the community

Works for me.

pdrake’s picture

pdrake CreditAttribution: pdrake commented
Status: Reviewed & tested by the community » Needs review
FileSize
webform-fix_infinite_loop-1804858-9.patch513 bytes

Ok, I noticed one more spacing problem. This patch resolves that.

bxtaylor’s picture

bxtaylor CreditAttribution: bxtaylor commented
Status: Needs review » Reviewed & tested by the community

Ah yes...that space on the return line...

Looks good to me, now.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Reviewed & tested by the community » Fixed

Thanks guys. Committed (finally)! Added to all 3.x and 4.x branches.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.