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 Bug
    • Status: Closed
    • Priority: Critical 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
    • Rank:
      18008

      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.

        Issue Links

          Activity

          Hide
          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
          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
          Dan Marsden added a comment -

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

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

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

          Show
          Dan Marsden added a comment - Linking to display issues that need to be fixed before we can push this into core.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Dan Marsden added a comment -

          rebased against latest branches

          Show
          Dan Marsden added a comment - rebased against latest branches
          Hide
          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
          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
          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
          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
          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
          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
          Dan Marsden added a comment -

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

          thanks.

          Show
          Dan Marsden added a comment - updated as requested and have re-submitted. (still not sure about your duplicate optional_param comment) thanks.
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

          reviewing this again....

          Show
          Eloy Lafuente (stronk7) added a comment - reviewing this again....
          Hide
          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
          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
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

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

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

          thanks Eloy!

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

          Thanks guys, passing this test now.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks guys, passing this test now. Cheers Sam
          Hide
          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
          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
          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
          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
          Eloy Lafuente (stronk7) added a comment -

          Sent upstream and closing, many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Sent upstream and closing, many thanks!
          Hide
          Hamid Haghayegh added a comment -

          Can we have this patch for moodle 1.9.x?

          Thanks
          H

          Show
          Hamid Haghayegh added a comment - Can we have this patch for moodle 1.9.x? Thanks H
          Hide
          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
          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: