Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-28295

SCORM crashing in IE 7 and above when httpreq is called under certain conditions when using SCORM popups

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.0, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: SCORM
    • Labels:
    • Testing Instructions:
      Hide

      Testing the IE 7 issue is a real pain and isn't advised if you want to keep some form of sanity about you..

      but... you can test the pop-up new window stuff.
      Create a new SCORM - if you don't have one - here's one:
      http://moodle.org/mod/data/view.php?d=50&rid=1654
      in the SCORM settings - choose to open the SCORM in a new window (might be hidden in advanced settings)
      Experiment with the option "display toc" and see if all the options work as expected in the new window
      Make sure the player uses the full window available - if you resize the window the player should expand both height and width to use the available space,

      Show
      Testing the IE 7 issue is a real pain and isn't advised if you want to keep some form of sanity about you.. but... you can test the pop-up new window stuff. Create a new SCORM - if you don't have one - here's one: http://moodle.org/mod/data/view.php?d=50&rid=1654 in the SCORM settings - choose to open the SCORM in a new window (might be hidden in advanced settings) Experiment with the option "display toc" and see if all the options work as expected in the new window Make sure the player uses the full window available - if you resize the window the player should expand both height and width to use the available space,
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull Master Branch:
      master_MDL-28295

      Description

      when dorequest is passed a url like this:
      /mod/scorm/datamodel.php?id=&a=11&cmi_corelesson_status=incomplete&cmicoreexit=suspend&cmi_suspend_data=|&attempt=1&scoid=28

      it fails.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            danmarsden Dan Marsden added a comment -

            pretty sure this is because the params passed aren't encoded correctly - most browsers don't care, but it seems IE does!

            Show
            danmarsden Dan Marsden added a comment - pretty sure this is because the params passed aren't encoded correctly - most browsers don't care, but it seems IE does!
            Hide
            danmarsden Dan Marsden added a comment -

            updated code - using full player.php page to load popup instead which appears to fix issue.

            Show
            danmarsden Dan Marsden added a comment - updated code - using full player.php page to load popup instead which appears to fix issue.
            Hide
            danmarsden Dan Marsden added a comment -

            Linking to display issues that need to be fixed before we can push this into core.

            Show
            danmarsden Dan Marsden added a comment - Linking to display issues that need to be fixed before we can push this into core.
            Hide
            danmarsden Dan Marsden added a comment -

            I wonder if we should check if SCORM TOC is disabled and if so - don't use the full player but use a standard object/frame include.

            Show
            danmarsden Dan Marsden added a comment - I wonder if we should check if SCORM TOC is disabled and if so - don't use the full player but use a standard object/frame include.
            Hide
            danmarsden Dan Marsden added a comment -

            note: the patch for master on this also includes the patch for MDL-28493 to help avoid conflicts during integration. MDL-28493 must be merged first.

            Show
            danmarsden Dan Marsden added a comment - note: the patch for master on this also includes the patch for MDL-28493 to help avoid conflicts during integration. MDL-28493 must be merged first.
            Hide
            danmarsden Dan Marsden added a comment - - edited

            Note: I'm only pushing in a fix for 2.X branches at this stage - If someone else wants to take my patch and develop a 1.9 version I'd be happy to review it for inclusion but I have no plans to do it myself (unless someone pays me for the time to do it!)

            This patch also disables the TOC and navbar for existing SCORMs that open in a new window as if these appeared for existing scorms in a new window it would probably cause great confusion!

            Show
            danmarsden Dan Marsden added a comment - - edited Note: I'm only pushing in a fix for 2.X branches at this stage - If someone else wants to take my patch and develop a 1.9 version I'd be happy to review it for inclusion but I have no plans to do it myself (unless someone pays me for the time to do it!) This patch also disables the TOC and navbar for existing SCORMs that open in a new window as if these appeared for existing scorms in a new window it would probably cause great confusion!
            Hide
            danmarsden Dan Marsden added a comment -

            note to integrator - I've just updated those branches with a savepoint function typo fix like the attached bug - whoops!

            Show
            danmarsden Dan Marsden added a comment - note to integrator - I've just updated those branches with a savepoint function typo fix like the attached bug - whoops!
            Hide
            danmarsden Dan Marsden added a comment -

            rebased against latest branches

            Show
            danmarsden Dan Marsden added a comment - rebased against latest branches
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Ops, failing integration of this because:

            1) Critical: I get some obvious duplicate code, for example @ mod/scorm/player.php - all the optional_param() seem duplicate. Haven't verified the rest.
            2) Not critical: Upgrade script, perhaps better if limiting the fields retrieved from DB to the used ones and/or use recordset? Imagine site with 20.000 scorms in popups and big summaries or so. I know it's an extreme situation, but...
            3) Trivial: Not knowing much about CSS... it surprised me the use of "font-size: medium", I thought we were using always em or friends.
            4) Question: 20_STABLE and 21_STABLE have the same version? Does that mean that are interchangeable? In other words... does the 21_STABLE version work if installed into 20_STABLE site?
            5) Side comment: While reviewing the code, it seems that the master version is 99% about to fix the popup thingy. Seems 100% unrelated with the title of the issue and that annoyed me a bit.
            6) Testing instructions of some type would be great if possible (I know, some things are hardly testable... but the popup thing in code seems to be).

            Thanks and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Ops, failing integration of this because: 1) Critical: I get some obvious duplicate code, for example @ mod/scorm/player.php - all the optional_param() seem duplicate. Haven't verified the rest. 2) Not critical: Upgrade script, perhaps better if limiting the fields retrieved from DB to the used ones and/or use recordset? Imagine site with 20.000 scorms in popups and big summaries or so. I know it's an extreme situation, but... 3) Trivial: Not knowing much about CSS... it surprised me the use of "font-size: medium", I thought we were using always em or friends. 4) Question: 20_STABLE and 21_STABLE have the same version? Does that mean that are interchangeable? In other words... does the 21_STABLE version work if installed into 20_STABLE site? 5) Side comment: While reviewing the code, it seems that the master version is 99% about to fix the popup thingy. Seems 100% unrelated with the title of the issue and that annoyed me a bit. 6) Testing instructions of some type would be great if possible (I know, some things are hardly testable... but the popup thing in code seems to be). Thanks and ciao
            Hide
            danmarsden Dan Marsden added a comment -

            1 - not sure what duplicate you mean here eloy the optional_params in player.php are all for different things.
            2 - Update script - sure, will tidy this up a bit.
            3 - ok will update
            4 - 20_stable and 21_stable branches are both very close so the patch should be interchangeable if you want - not sure why you're asking this.
            5 - the popup thing has changed because of the IE 7 madness - we can't load the SCO in a new window and keep the api in the parent window as for some stupid reason IE 7 fails intermittently.
            6 - fair enough - the IE 7 issue is a real pain to reproduce but the pop-ups can be tested... any chance of getting this fixed this week if I make the above changes today?

            Show
            danmarsden Dan Marsden added a comment - 1 - not sure what duplicate you mean here eloy the optional_params in player.php are all for different things. 2 - Update script - sure, will tidy this up a bit. 3 - ok will update 4 - 20_stable and 21_stable branches are both very close so the patch should be interchangeable if you want - not sure why you're asking this. 5 - the popup thing has changed because of the IE 7 madness - we can't load the SCO in a new window and keep the api in the parent window as for some stupid reason IE 7 fails intermittently. 6 - fair enough - the IE 7 issue is a real pain to reproduce but the pop-ups can be tested... any chance of getting this fixed this week if I make the above changes today?
            Hide
            danmarsden Dan Marsden added a comment -

            player.php
            $mode == mode the SCORM should be loaded in - normal, review, browse etc. (as per SCORM Spec)
            $newattempt == the user has selected to start a new attempt.
            it shuold be obvious what the other optional_params are used for.

            Show
            danmarsden Dan Marsden added a comment - player.php $mode == mode the SCORM should be loaded in - normal, review, browse etc. (as per SCORM Spec) $newattempt == the user has selected to start a new attempt. it shuold be obvious what the other optional_params are used for.
            Hide
            danmarsden Dan Marsden added a comment -

            updated as requested and have re-submitted. (still not sure about your duplicate optional_param comment)

            thanks.

            Show
            danmarsden Dan Marsden added a comment - updated as requested and have re-submitted. (still not sure about your duplicate optional_param comment) thanks.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited

            Take a look to: https://github.com/danmarsden/moodle/blob/f9ce4a21c06d50711c7ecf00568be76c4dc74259/mod/scorm/player.php

            or I'm blind or I'm seeing a lot of optional/required_param() dupe lines there. Sure it's my eyes getting tired lol... so better I go to sleep some hours... see you tomorrow, and thanks!

            And yes, I'll re-try this tomorrow...ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - - edited Take a look to: https://github.com/danmarsden/moodle/blob/f9ce4a21c06d50711c7ecf00568be76c4dc74259/mod/scorm/player.php or I'm blind or I'm seeing a lot of optional/required_param() dupe lines there. Sure it's my eyes getting tired lol... so better I go to sleep some hours... see you tomorrow, and thanks! And yes, I'll re-try this tomorrow...ciao
            Hide
            danmarsden Dan Marsden added a comment -

            whoops! - I was only looking at the master branch just now - yeah that's a bad copy/paste... I'll check the full patches for 2.0Stable and 2.1Stable again! - sorry bout that!

            Show
            danmarsden Dan Marsden added a comment - whoops! - I was only looking at the master branch just now - yeah that's a bad copy/paste... I'll check the full patches for 2.0Stable and 2.1Stable again! - sorry bout that!
            Hide
            danmarsden Dan Marsden added a comment -

            pushed updated code for 2.0 and 2.1 stable branches - I've also removed a lot of the whitespace clean up stuff to hopefully make it easier to review! - sorry for pushing such a messy commit!

            thanks!

            Show
            danmarsden Dan Marsden added a comment - pushed updated code for 2.0 and 2.1 stable branches - I've also removed a lot of the whitespace clean up stuff to hopefully make it easier to review! - sorry for pushing such a messy commit! thanks!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Thanks Dan, re-looking to this now.

            About 4) in my original list... what I was asking is that, as far as 20_STABLE, 21_STABLE and master branches, all them have "$module->requires = 2010080300;" then, potentially, it's possible to run the 21_STABLE module into one 20_STABLE site (and also one master module into one 20 or 21_STABLE site).

            And I was thinking that perhaps, although improbable... that was not a good thing. Just that. As far as you keep 20_STABLE and 21_STABLE fully in sync, np here, but if they diverge, using some new stuff only present in 21_STABLE... you should update that "$module->requires" to point to newer release of Moodle.

            Not critical at all, it just surprised me you were using the same version numbers in the 2 branches. Ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Thanks Dan, re-looking to this now. About 4) in my original list... what I was asking is that, as far as 20_STABLE, 21_STABLE and master branches, all them have "$module->requires = 2010080300;" then, potentially, it's possible to run the 21_STABLE module into one 20_STABLE site (and also one master module into one 20 or 21_STABLE site). And I was thinking that perhaps, although improbable... that was not a good thing. Just that. As far as you keep 20_STABLE and 21_STABLE fully in sync, np here, but if they diverge, using some new stuff only present in 21_STABLE... you should update that "$module->requires" to point to newer release of Moodle. Not critical at all, it just surprised me you were using the same version numbers in the 2 branches. Ciao
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            reviewing this again....

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - reviewing this again....
            Hide
            danmarsden Dan Marsden added a comment -

            cool - thanks Eloy - 2.0 and 2.1 should be in sync - anything different will only be in master, anything pushed to 2.1stable will also go to 2.0stable... I figure anything that can't be pushed to 2.0stable shouldn't go to 2.1stable either so I'll probably keep them both in sync for a while.

            the only stuff I've been pushing into stable branches for SCORM are bug fixes excluding any SCORM 2004 stuff which is only going in master.

            thanks heaps!

            Show
            danmarsden Dan Marsden added a comment - cool - thanks Eloy - 2.0 and 2.1 should be in sync - anything different will only be in master, anything pushed to 2.1stable will also go to 2.0stable... I figure anything that can't be pushed to 2.0stable shouldn't go to 2.1stable either so I'll probably keep them both in sync for a while. the only stuff I've been pushing into stable branches for SCORM are bug fixes excluding any SCORM 2004 stuff which is only going in master. thanks heaps!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Integrated, thanks!

            Note for tester: Plz, test this under some STABLE branch.

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Integrated, thanks! Note for tester: Plz, test this under some STABLE branch.
            Hide
            danmarsden Dan Marsden added a comment -

            thanks Eloy!

            Show
            danmarsden Dan Marsden added a comment - thanks Eloy!
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks guys, passing this test now.

            Cheers
            Sam

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks guys, passing this test now. Cheers Sam
            Hide
            gblair Gabe Blair added a comment - - edited

            Hi Dan,

            Sorry for double-posting (here and in the forums) - I should rather have posted this only here, probably. I applied your patch to 2.0.4, and it seems to fix IE7 - awesome! However, the same popup still freezes in IE8. Do you have any insight? Maybe I applied the path wrongly...? Here's what I did:

            1. Clone the git repo at git://github.com/danmarsden/moodle.git as a standalone set of files (not on top of my Moodle instance)
            2. Checkout MOODLE_20_STABLE branch and use it to upgrade my Moodle instance, by replacing everything with the new set of files, then overlaying my custom auth and theme folders, and config.php
            3. Checkout the m20_MDL-28295 branch (your fix) to the standalone location, and copy the entire mod/scorm/ directory to my Moodle instance

            This appears to have fixed the popup issue in IE7, but in IE8, the popup window still hangs at load time, crashing the browser.

            Thanks for any insight you can provide!

            Show
            gblair Gabe Blair added a comment - - edited Hi Dan, Sorry for double-posting (here and in the forums) - I should rather have posted this only here, probably. I applied your patch to 2.0.4, and it seems to fix IE7 - awesome! However, the same popup still freezes in IE8. Do you have any insight? Maybe I applied the path wrongly...? Here's what I did: 1. Clone the git repo at git://github.com/danmarsden/moodle.git as a standalone set of files (not on top of my Moodle instance) 2. Checkout MOODLE_20_STABLE branch and use it to upgrade my Moodle instance, by replacing everything with the new set of files, then overlaying my custom auth and theme folders, and config.php 3. Checkout the m20_ MDL-28295 branch (your fix) to the standalone location, and copy the entire mod/scorm/ directory to my Moodle instance This appears to have fixed the popup issue in IE7, but in IE8, the popup window still hangs at load time, crashing the browser. Thanks for any insight you can provide!
            Hide
            danmarsden Dan Marsden added a comment -

            Hi Gabe,

            good to hear! - I haven't done any testing of IE 8 yet but we do have this:
            http://tracker.moodle.org/browse/MDL-28551

            try the fix posted in the forum and see if that resolves it for you? - if not we'll need lots of information on how to reproduce it - including SCORM object, IIS/Apache (what versions) php version, what php extensions are installed etc etc.

            thanks!

            Show
            danmarsden Dan Marsden added a comment - Hi Gabe, good to hear! - I haven't done any testing of IE 8 yet but we do have this: http://tracker.moodle.org/browse/MDL-28551 try the fix posted in the forum and see if that resolves it for you? - if not we'll need lots of information on how to reproduce it - including SCORM object, IIS/Apache (what versions) php version, what php extensions are installed etc etc. thanks!
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Sent upstream and closing, many thanks!

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Sent upstream and closing, many thanks!
            Hide
            hamid.haghayegh@gmail.com Hamid Haghayegh added a comment -

            Can we have this patch for moodle 1.9.x?

            Thanks
            H

            Show
            hamid.haghayegh@gmail.com Hamid Haghayegh added a comment - Can we have this patch for moodle 1.9.x? Thanks H
            Hide
            danmarsden Dan Marsden added a comment -

            Hi Hamid,

            not something I plan to do sorry - although you may not need it - have replied to you on your other post.

            Show
            danmarsden Dan Marsden added a comment - Hi Hamid, not something I plan to do sorry - although you may not need it - have replied to you on your other post.

              People

              • Votes:
                4 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Oct/11