? Success

User tests: Successful: Unsuccessful:

avatar roland-d
roland-d
28 Jan 2016

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 :)

avatar roland-d roland-d - open - 28 Jan 2016
avatar roland-d roland-d - change - 28 Jan 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 28 Jan 2016
Labels Added: ?
avatar mbabker
mbabker - comment - 28 Jan 2016

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)

avatar roland-d
roland-d - comment - 28 Jan 2016

Thank you @mbabker Code has been updated.

avatar brianteeman brianteeman - test_item - 28 Jan 2016 - Tested successfully
avatar brianteeman
brianteeman - comment - 28 Jan 2016

I have tested this item :white_check_mark: 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.

avatar richard67
richard67 - comment - 28 Jan 2016

@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:

screen shot 2016-01-28 at 10 55 22

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.

avatar richard67
richard67 - comment - 28 Jan 2016

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.

avatar roland-d
roland-d - comment - 28 Jan 2016

@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.

avatar richard67 richard67 - test_item - 28 Jan 2016 - Tested successfully
avatar richard67
richard67 - comment - 28 Jan 2016

I have tested this item :white_check_mark: 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.

avatar mbabker
mbabker - comment - 28 Jan 2016

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.

avatar roland-d
roland-d - comment - 28 Jan 2016

Hello Michael,

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

image

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.

avatar richard67
richard67 - comment - 28 Jan 2016

@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.

avatar mbabker
mbabker - comment - 28 Jan 2016

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).

avatar Kubik-Rubik Kubik-Rubik - test_item - 29 Jan 2016 - Tested successfully
avatar Kubik-Rubik
Kubik-Rubik - comment - 29 Jan 2016

I have tested this item :white_check_mark: successfully on bbb6f33


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

avatar Kubik-Rubik Kubik-Rubik - change - 29 Jan 2016
Status Pending Ready to Commit
avatar Kubik-Rubik Kubik-Rubik - change - 29 Jan 2016
Labels Added: ?
avatar Kubik-Rubik Kubik-Rubik - change - 29 Jan 2016
Milestone Added:
avatar infograf768
infograf768 - comment - 29 Jan 2016

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

Warning
Your session has expired. Please log in again.

avatar brianteeman
brianteeman - comment - 29 Jan 2016

@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

avatar roland-d
roland-d - comment - 29 Jan 2016

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

avatar Kubik-Rubik
Kubik-Rubik - comment - 29 Jan 2016

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

avatar Kubik-Rubik Kubik-Rubik - close - 29 Jan 2016
avatar joomla-cms-bot joomla-cms-bot - close - 29 Jan 2016
avatar Kubik-Rubik Kubik-Rubik - reference | 65d66a6 - 29 Jan 16
avatar Kubik-Rubik Kubik-Rubik - merge - 29 Jan 2016
avatar Kubik-Rubik Kubik-Rubik - close - 29 Jan 2016
avatar Kubik-Rubik Kubik-Rubik - change - 29 Jan 2016
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2016-01-29 09:23:15
Closed_By Kubik-Rubik
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 29 Jan 2016
Labels Removed: ?
avatar brianteeman
brianteeman - comment - 29 Jan 2016

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

avatar Bodge-IT
Bodge-IT - comment - 30 Jan 2016

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.

avatar brianteeman
brianteeman - comment - 30 Jan 2016

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

avatar Bodge-IT
Bodge-IT - comment - 30 Jan 2016

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

avatar brianteeman
brianteeman - comment - 30 Jan 2016

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/

avatar Bodge-IT
Bodge-IT - comment - 30 Jan 2016

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

avatar brianteeman
brianteeman - comment - 30 Jan 2016

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/

Add a Comment

Login with GitHub to post a comment