New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"Infinite loop attempting to show error page" for non existing group profile page #9476

Closed
jeabakker opened this Issue Mar 11, 2016 · 8 comments

Comments

Projects
None yet
3 participants
@jeabakker
Member

jeabakker commented Mar 11, 2016

when attempting to view /groups/profile/83193874912374912376 you'll get the message "Infinite loop attempting to show error page" instead of the (expected) 404 page

reason is https://github.com/Elgg/Elgg/blob/2.0/mod/groups/start.php#L162 which triggers (for the 2nd time) the error page handler

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 11, 2016

Contributor

Oh, that's a nice one. Can I just say out loud once again that pagesetup needs to die.

Contributor

hypeJunction commented Mar 11, 2016

Oh, that's a nice one. Can I just say out loud once again that pagesetup needs to die.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 11, 2016

Member

Not a problem in 2.x. Hmm.

Member

mrclay commented Mar 11, 2016

Not a problem in 2.x. Hmm.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 11, 2016

Member

So #9440 has introduced this bug. Hell.

Member

mrclay commented Mar 11, 2016

So #9440 has introduced this bug. Hell.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 11, 2016

Member

Well, more general problem is we allow re-triggering system events. Not what's happening.

Member

mrclay commented Mar 11, 2016

Well, more general problem is we allow re-triggering system events. Not what's happening.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 11, 2016

Member

In 1.12, 404 forwards were allowed to nest, going through elgg_error_page_handler() multiple times, basically the last one "winning" to draw the page.

This was dangerous, but I didn't run into an infinite loop until doing the views overhaul in 2.0. At which point I (wrongly) assumed even twice was a sign of an infinite loop and I added the abrupt exit.

After #9440, the pagesetup is no longer triggered by the resource/group/profile page, so the page resource does the first 404, the error page triggers pagesetup, and the pagesetup handler for group triggers the 2nd 404.

A simple fix would be to allow maybe 3 calls to elgg_error_page_handler() before pulling the plug.

Member

mrclay commented Mar 11, 2016

In 1.12, 404 forwards were allowed to nest, going through elgg_error_page_handler() multiple times, basically the last one "winning" to draw the page.

This was dangerous, but I didn't run into an infinite loop until doing the views overhaul in 2.0. At which point I (wrongly) assumed even twice was a sign of an infinite loop and I added the abrupt exit.

After #9440, the pagesetup is no longer triggered by the resource/group/profile page, so the page resource does the first 404, the error page triggers pagesetup, and the pagesetup handler for group triggers the 2nd 404.

A simple fix would be to allow maybe 3 calls to elgg_error_page_handler() before pulling the plug.

@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 11, 2016

Member

But generally we should also decide what to do when code wants to draw an error page during the process of drawing the error page.

Member

mrclay commented Mar 11, 2016

But generally we should also decide what to do when code wants to draw an error page during the process of drawing the error page.

@hypeJunction

This comment has been minimized.

Show comment
Hide comment
@hypeJunction

hypeJunction Mar 12, 2016

Contributor

We can add proper page handlers for error codes and actually redirect to
them rather than drawing the page within a page.
On Mar 12, 2016 12:20 AM, "Steve Clay" notifications@github.com wrote:

In 1.12, 404 forwards were allowed to nest, going through
elgg_error_page_handler() multiple times, basically the last one "winning"
to draw the page.

This was dangerous, but I didn't run into an infinite loop until doing the
views overhaul in 2.0. At which point I (wrongly) assumed even twice was a
sign of an infinite loop and I added the abrupt exit.

After #9440 #9440, the pagesetup is no
longer triggered by the resource/group/profile page, so the page resource
does the first 404, the error page triggers pagesetup, and the pagesetup
handler for group triggers the 2nd 404.

A simple fix would be to allow maybe 3 calls to elgg_error_page_handler()
before pulling the plug.


Reply to this email directly or view it on GitHub
#9476 (comment).

Contributor

hypeJunction commented Mar 12, 2016

We can add proper page handlers for error codes and actually redirect to
them rather than drawing the page within a page.
On Mar 12, 2016 12:20 AM, "Steve Clay" notifications@github.com wrote:

In 1.12, 404 forwards were allowed to nest, going through
elgg_error_page_handler() multiple times, basically the last one "winning"
to draw the page.

This was dangerous, but I didn't run into an infinite loop until doing the
views overhaul in 2.0. At which point I (wrongly) assumed even twice was a
sign of an infinite loop and I added the abrupt exit.

After #9440 #9440, the pagesetup is no
longer triggered by the resource/group/profile page, so the page resource
does the first 404, the error page triggers pagesetup, and the pagesetup
handler for group triggers the 2nd 404.

A simple fix would be to allow maybe 3 calls to elgg_error_page_handler()
before pulling the plug.


Reply to this email directly or view it on GitHub
#9476 (comment).

mrclay added a commit to mrclay/Elgg-leaf that referenced this issue Mar 12, 2016

fix(errors): nested forward 404 calls are less likely to abruptly fail
Sometimes while drawing the error page, plugin code may request the error
page again (often code within the `pagesetup` event). We no longer fail
with an empty page on the 2nd call since this is more common that expected.

On the 3rd call we assume an infinite loop and just use a Location header
to send the user to the home page.

Fixes #9476
@mrclay

This comment has been minimized.

Show comment
Hide comment
@mrclay

mrclay Mar 12, 2016

Member

This might work for 2.0 #9477

Member

mrclay commented Mar 12, 2016

This might work for 2.0 #9477

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment