[Gnash-commit] [patch #9279] Patch to fix wrong behavior in getURL()/MovieClip.getURL() on `undefined` and `null` target

[Nutchanon Wetchasit]

URL:
<http://savannah.gnu.org/patch/?9279>

Summary: Patch to fix wrong behavior in
getURL()/MovieClip.getURL() on `undefined` and `null` target
Project: Gnash - The GNU Flash player
Submitted by: nachanon
Submitted on: Sun 05 Mar 2017 06:37:12 PM ICT
Category: core
Priority: 5 - Normal
Status: None
Privacy: Public
Assigned to: None
Originator Email:
Open/Closed: Open
Discussion Lock: Any

_______________________________________________________

Details:

This patch series attempts to fix bug #50393 (undefined/null
getURL()/MovieClip.getURL() target problem) and few issues around them.

Patch 1 of 6:
See the attached `0001_add-builtin-fscommand-parameter-tests.patch`.

This patch adds automated checks on built-in `getURL()`-based FSCommand usage
with mutltiple kind of call name and parameters to Gnash testsuite. These
checks are added into MakeSWF-based `hostcmd` test (in both container-emulated
and HTML mode).

Like the existing checks, they run on SWF version 5 to 8 (no logic for SWF4
yet), non-predefined FSCommand(s) are used for implementation simplicity, and
XFAILED checks are used when Gnash's behavior deviates from Flash Player.

Gnash: 0.8.11dev (patched against git 144e082 22-Feb-2017)
Adobe Flash Player: 11.2.202.491 (NPAPI binary)
Browser: Iceweasel 10.0.12 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386




_______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Sun 05 Mar 2017 06:37:12 PM ICT Name:
0001_add-builtin-fscommand-parameter-tests.patch Size: 36kB By: nachanon
Patch to add automated tests on getURL()-based FSCommand
<http://savannah.gnu.org/patch/download.php?file_id=39889>

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9279>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Nutchanon Wetchasit]

Follow-up Comment #1, patch #9279 (project gnash):

Patch 2 of 6:
See the attached `0002_correct-movieclip-geturl-undefined-target.patch`.

This patch makes Gnash's implementation of `MovieClip.getURL()`
<https://git.savannah.gnu.org/cgit/gnash.git/tree/libcore/asobj/MovieClip_as.cpp?id=144e0827072b746b2bedebe39ca8a7e0fb43a455#n1089>
treat `undefined` target parameter as if it was an empty string; like Flash
Player always does, no matter of SWF version
<https://savannah.gnu.org/bugs/?50393#comment1>.

Corresponding 2 XFAILED checks in Gnash testsuite are changed to PASSED in
this patch; resulting in more 8 PASSED checks total when counted all SWF
version and test method. (4 more PASSED in automated run, and 4 more PASSED in
browser-based run)

Gnash: 0.8.11dev (patched against git 144e082 22-Feb-2017)
Adobe Flash Player: 11.2.202.491 (NPAPI binary)
Browser: Iceweasel 10.0.12 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386


(file #39890)
_______________________________________________________

Additional Item Attachment:

File name: 0002_correct-movieclip-geturl-undefined-target.patch Size:3 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9279>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Nutchanon Wetchasit]

Follow-up Comment #2, patch #9279 (project gnash):

Patch 3 of 6:
See the attached `0003_correct-builtin-geturl-undefined-null-target.patch`.

Current Gnash treats `undefined` and `null` target parameter in built-in
`getURL()` function as if they were empty string
<https://git.savannah.gnu.org/cgit/gnash.git/tree/libcore/vm/ASHandlers.cpp?id=144e0827072b746b2bedebe39ca8a7e0fb43a455#n3524>.

Flash Player, other the other hand, uses (SWF version-specific) string
representation of the parameter
<https://savannah.gnu.org/bugs/?50393#comment1> as target. This behavior is
the same in both normal and FSCommand usage.

This patch fixes Gnash implementation by replacing the special case treatment
with SWF version-specific string conversion.

Corresponding 4 XFAILED check in Gnash testsuite are changed to PASSED in this
patch; resulting in 16 more PASSED checks total when counted all SWF version
and test method. (8 more PASSED in automated run, 8 more PASSED in
browser-based run)

Side note (for the record):
MakeSWF processes `getURL("loaded.html")` into ActionGetURL2 instruction that
looks more or less like `getURL("loaded.html","")`. Macromedia Flash MX 2004
also processes the call into a similar statement, though it uses an old-school
ActionGetURL instead of ActionGetURL2.

Gnash: 0.8.11dev (patched against git 144e082 22-Feb-2017)
Adobe Flash Player: 11.2.202.491 (NPAPI binary)
Browser: Iceweasel 10.0.12 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386


(file #39891)
_______________________________________________________

Additional Item Attachment:

File name: 0003_correct-builtin-geturl-undefined-null-target.patch Size:4 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9279>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Nutchanon Wetchasit]

Follow-up Comment #3, patch #9279 (project gnash):

Patch 4 of 6:
See the attached `0004_fscommand-add-function-parameter-tests.patch`.

Currently, `hostcmd` FSCommand test in Gnash testsuite does not cover
function-type parameter, this patch adds it in (for both container-emulated
mode and HTML mode).

Gnash: 0.8.11dev (patched against git 144e082 22-Feb-2017)
Adobe Flash Player: 11.2.202.491 (NPAPI binary)
Browser: Iceweasel 10.0.12 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386


(file #39892)
_______________________________________________________

Additional Item Attachment:

File name: 0004_fscommand-add-function-parameter-tests.patch Size:6 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9279>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Nutchanon Wetchasit]

Follow-up Comment #4, patch #9279 (project gnash):

Patch 5 of 6:
See the attached `0005_hostcmd-test-show-player-output.patch`.

Current container-emulated `hostcmd` test runner redirected player output away
to a log file and discarded it without ever printing it out to the main test
log.

This patch makes the test runner print out the Gnash-side player output,
enabling user to verify/debug it from `testrun.log`; like the original
`extcomm` test
<https://git.savannah.gnu.org/cgit/gnash.git/tree/testsuite/misc-mtasc.all/extcommtests-runner.sh?id=144e0827072b746b2bedebe39ca8a7e0fb43a455#n273>
do.

Gnash: 0.8.11dev (patched against git 144e082 22-Feb-2017)
System: Debian GNU/Linux 7.0 Wheezy i386


(file #39893)
_______________________________________________________

Additional Item Attachment:

File name: 0005_hostcmd-test-show-player-output.patch Size:0 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9279>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Nutchanon Wetchasit]

Follow-up Comment #5, patch #9279 (project gnash):

Patch 6 of 6:
See the attached `0006_geturl-add-weird-target-tests.patch`.

This patch adds automated checks non-FSCommand usage of `getURL()` and
`MovieClip.getURL()` when encountered with weird target parameter like empty
"", `null`, and `undefined` to Gnash testsuite; according to reference
behavior listed on bug #50393 comment 1
<https://savannah.gnu.org/bugs/?50393#comment1>.

Implementation notes:

* This test is based on MakeSWF, and run under shell-based container emulation
environment, with external termination (much like the `hostcmd` test).
* This test runs on SWF version 4 to 8 as `getURL()` is available since Flash
4 onward- according to Macromedia's documentation.
* I implemented this as a separate test runner from `hostcmd` as it cannot be
run under the same HTML-mode container.
* There's no XFAILED check, as problems are already fixed by earlier
<https://savannah.gnu.org/patch/?9279#comment1> patches
<https://savannah.gnu.org/patch/?9279#comment2>.

This patch does not provide HTML-based test runner, as it is beyond limit of
what simple semi-automated HTML test page could do. Also, implementing the
test even in semi-automated fashion would require another exotic Flash feature
(FlashVars), which must be tested first.

Gnash: 0.8.11dev (patched against git 144e082 22-Feb-2017)
System: Debian GNU/Linux 7.0 Wheezy i386


(file #39894)
_______________________________________________________

Additional Item Attachment:

File name: 0006_geturl-add-weird-target-tests.patch Size:15 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9279>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Update of patch #9279 (project gnash):

Status: None => In Progress
Assigned to: None => strk


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9279>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #6, patch #9279 (project gnash):

I'm blocked in testing by bug #50478 - will get back here as soon as that one
is fixed

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9279>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Update of patch #9279 (project gnash):

Status: In Progress => Done
Open/Closed: Open => Closed

_______________________________________________________

Follow-up Comment #7:

All pushed as of commit bed373ff164acbe8ac56ee340714ed00ce1c3aca


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?9279>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/