Fixed fatal error message on session timeout

[roland-d]

This is a redo of PR #8808 based on the feedback by @mbabker

Test instructions

1. In the global configuration set the timeout to 1 minute
2. Go back to the administrator control panel
3. Wait 2 minutes
4. Click on any link
5. You are logged out and a fatal error is shown about the session
6. Apply the patch
7. Repeat steps 2 - 4
8. You are now logged out and no fatal error is shown

A signal for @wilsonge :)

[mbabker]

Two comments:

1. Remove the @throws line in doc block
2. Change the // If running PHP 5.4, try to use the native API comment to explain what is actually being checked (you're verifying the session is actually active)

[roland-d]

Thank you @mbabker Code has been updated.

[brianteeman]

I have tested this item ✅ successfully on bbb6f33

Tested on php 5.5.26 the fatal error has now gone

But I still have to login twice and am not returned to the page I was on when the session timed out

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9011.}

[richard67]

@roland-d I have tested this successfully as described above in test instructions, BUT:

When updating from 3.4.8 using a patched Beta 2 update container wia Joomla Update (custom URL with patched XML), the update ends with following display:

And the error log file of PHP contains following error:

[28-Jan-2016 17:43:28 Europe/Berlin] PHP Fatal error:  Uncaught exception 'RuntimeException' with message 'The session is not started.' in /homepages/xx/yy/htdocs/test1/libraries/joomla/session/handler/native.php:221
Stack trace:
#0 [internal function]: JSessionHandlerNative->save()
#1 {main}
  thrown in /homepages/xx/yy/htdocs/test1/libraries/joomla/session/handler/native.php on line 221

Please advice if I shall set my test result to "success" because this is another issue or cannot be avoided when upgrading from a pre-3.5, or to "failed" because you wanna fix that here, too.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9011.}

[richard67]

P.S.: My PHP version is 5.6.15

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9011.}

[roland-d]

@richard67 That error comes from the old native.php file. Line 221 only has a curly brace on it once this patch is applied.

A timeout during update, sounds odd though. Perhaps that is another issue. Your test is good I would say, since we need to test with the new native.php file.

[richard67]

I have tested this item ✅ successfully on bbb6f33

Tested with success as described in test instructions, PHP version used for the test was 5.6.15.
Unfortunately I have no PHP 5.3 anymore so I cannot test for this, but from my point of view it would make sense to have a good test also for PHP 5.3.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9011.}

[mbabker]

All that points to is what I was saying in the last pull request; something is trying to start/shutdown the session more than the expected once per request cycle. That error specifically isn't related to this pull request, but it needs to be dug into. If someone who is experiencing these issues can open a new bug report and include a stack trace (the debug_backtrace() function will do the trick) showing the paths that are causing JSession::_start() and JSession::close() to be called would be helpful. From a cursory glance, there may be a path where JSession::restart(), JSession::destroy(), or JSession::fork() get called that may be able to cause a glitch but it's just hypothetical at the moment.

[roland-d]

Hello Michael,

When I debugged this, the stack trace wouldn't show anything, it just shows this:

When I step through it I went from

$app = JFactory::getApplication('administrator'); 

in index.php to

if (!$this->_validate()) 

in session.php. The validation comes back as false because more than 60 seconds has passed in my case. This then calls:

$this->destroy();

in session.php which ends up at the save() method in native.php because that is registered as shutdown function.

Since the session hasn't started (it was destroyed earlier) it now fails the check:

if (!$this->isStarted())

in native.php, throwing out the RuntimeException.

That is what I dug up.

[richard67]

@mbabker See also my error log above with the problem on update from 3.4.8 and the new native.php: A stack trace was printed out, but is shows only the 1 frame for JSessionHandlerNative->save(). And I am not familiar with debug_backtrace(), so I have the issue but am not sure if I can be the one who digs into this deeper. But if you want I can at least open an issue for that in the issue tracker.

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9011.}

[mbabker]

When something is triggered as a shutdown function it won't have a stack trace. So if the first occurrence is at that point then the shutdown handler shouldn't be the issue unless there's a bug in the shutdown workflow in general. This patch is fine specifically for the error about writing the session data when the session isn't started (according to Joomla's state). But there still seems to be some deep rooted logic error if the scenario @richard67 tested results in trying to start the session multiple times (meaning there's a disconnect between Joomla's internal state tracking and PHP's own active status).

[Kubik-Rubik]

I have tested this item ✅ successfully on bbb6f33

---

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9011.}

[infograf768]

Fatal error is gone here but I still have to log twice here.

Warning
Your session has expired. Please log in again.

[brianteeman]

@roland-d is this PR supposed to fix the double login and the regression to not be returned to the page you were on when the session timed out or just the fatal error

[roland-d]

@brianteeman This PR is only for fixing the fatal error.

[Kubik-Rubik]

Thank you @roland-d! I will merge this for now but we should take a deeper look on the issue that @mbabker raised.

[brianteeman]

thanks @roland-d I created a separate issue for that #9016

[Bodge-IT]

This PR supposed to be merged but just tested and verified issue without patching.
Patch not available to apply. Still new at this so will await TeamJUGL verification.

[brianteeman]

It has been merged so it is no longer present in the list of patches displayed by com_patchtester

[Bodge-IT]

Correct but the issue remained on Joomla downloaded from Github this morning

[brianteeman]

Yes it will be - but if you look it says merged

On 30 January 2016 at 10:48, Gary Barclay notifications@github.com wrote:

Correct but the issue remained on Joomla download from Github this morning

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

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/

[Bodge-IT]

Doesn't that mean the patch is applied to the staging version?

[brianteeman]

yes

On 30 January 2016 at 10:50, Gary Barclay notifications@github.com wrote:

Doesn't that mean the patch is applied to the staging version?

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

Brian Teeman
Co-founder Joomla! and OpenSourceMatters Inc.
http://brian.teeman.net/