patchGnash - The GNU Flash player - Patches: patch #8721, Patch to make `GetVariable()`...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

patch #8721: Patch to make `GetVariable()` return String regardless of original variable type

Submitted by:  Nutchanon Wetchasit <nachanon>
Submitted on:  Fri 21 Aug 2015 03:20:38 PM UTC  
 
Category: corePriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Sandro Santilli <strk>Open/Closed: Closed

(Jump to the original submission Jump to the original submission)

Mon 25 Jan 2016 04:34:50 PM UTC, comment #16:

So I'm closing this. Thanks.

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 25 Jan 2016 03:13:32 PM UTC, comment #15:

For the automated test case of this fix, see patch #8835 (committed as 48e75c1).

Nutchanon Wetchasit <nachanon>
Mon 07 Dec 2015 05:34:04 PM UTC, comment #14:

Oops, sorry for the noise, my last two comments (#12 and #13) were indeed intended to go to bug #46607 -- thanks Nutchanon for copying them over

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 07 Dec 2015 04:08:59 PM UTC, comment #13:

Also, _pos is never updated because there's a single buffer whose size() is < _in_point

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 07 Dec 2015 04:05:58 PM UTC, comment #12:

Bastiaan the consumeInPoint method in your change looks suspicious as you're setting _pos only at the final iteration of the loop, setting it to the advancement within the last buffer, rather than advancing all the buffers.

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 07 Dec 2015 03:56:50 PM UTC, comment #11:

I think we had a gnashrc setting for "ignorefscommand" which defaults to true when runnign as plugin but can be false while runinng the testsuite

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 07 Dec 2015 03:45:06 PM UTC, comment #10:

An alternative way for testing this would be adding a separate test runner
dedicated to `GetVariable()` test, that would run various version of test
SWF file, version 5 to version 8. Code of the test SWF would still be
more or less the same as getvariable-v3.

Implementing it this way will also cover bug #45840 (timeline-declared
undefined variable issue) as `MovieClip.hasOwnProperty()` test
does not work in SWF5. Downside of this approach would be the time consumed
by `make check`, unless we skip the "Gnash should terminate successfully" part
by making the test runner kill Gnash after it got all the answers.

P.S. `fscommand("quit")` is not supposed to work when SWF is running under
a plugin, which is the case as we run Gnash with `--fd` switch.

Nutchanon Wetchasit <nachanon>
Mon 07 Dec 2015 03:42:08 PM UTC, comment #9:

@strk:
No, patch #8770 (ac2b26f) included only basic test for Flash 8-style
ExternalInterface; built-in plugin function tests (e.g. `GetVariable()`)
are not included.

But since the pilot patch was accepted, adding more JavaScript-to-Flash
invocation test wouldn't be much of a hurdle. A test for this would likely be
based on the previously-posted getvariable-v3 test, and added directly
to `extcommtests-runner`. Limitation of this approach is the test be done
solely under SWF8 environment.

Nutchanon Wetchasit <nachanon>
Thu 03 Dec 2015 11:34:00 AM UTC, comment #8:

Nutchanon:

was a test for this included in the ExteenalInterface test case committed in ac2b26fd133b2fedf7f55c2e8e498a127907122a ?

Sandro Santilli <strk>
Project MemberIn charge of this item.
Fri 28 Aug 2015 06:58:38 PM UTC, comment #7:

The patch was committed as dc51f5389e4726486b631864151dcfe28e556cf6

Sandro Santilli <strk>
Project MemberIn charge of this item.
Fri 28 Aug 2015 06:49:31 PM UTC, comment #6:

I'd still love to see a testcase associated with this change, do you think you can provide that ?

Sandro Santilli <strk>
Project MemberIn charge of this item.
Fri 28 Aug 2015 01:16:04 PM UTC, comment #5:

For the behavior difference between Gnash and Flash, I have tested around
and found that this is not really caused by `as_object.get_member()`;
the function is actually working as advertised.

The problem seemed to root from the way Gnash deals with
unassigned `var` declaration in timeline context.

Excerpt from Gnash git's debug output (verbosity 2 + action error log)
when running `getvariable.html` test (file #34682):

This message is from the declaration of `var undefined_variable` used in
the test. This means that Gnash ignored the declaration and left the property
unset. Thus, `as_object.get_member()` reported `false` on the variable.

If the variable is declared with `var undefined_variable=undefined` instead,
`as_object.get_member()` will report the correct result.

This issue is now posted as a separate entry: bug #45840, test included.

If that bug is fixed, the proposed GetVariable() patch would automatically
give a correct output.

Gnash: 0.8.11dev (patched against git f341372 25-Aug-2015) NPAPI
Browser: Iceweasel 10.0.12 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386

(file #34761)

Nutchanon Wetchasit <nachanon>
Fri 28 Aug 2015 12:50:57 PM UTC, comment #4:

See the attached `0001_return-string-from-getvariable-v2.patch`
for original patch with more specific wording in the documentation.

(file #34759)

Nutchanon Wetchasit <nachanon>
Tue 25 Aug 2015 05:23:40 PM UTC, comment #3:

See also plugin/npapi/test.cpp for a possible place to write tests for this patch.

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 24 Aug 2015 10:49:57 AM UTC, comment #2:

The bool as_object::get_member method is documented to return false when the variable isn't found (rather than is set as undefined), and your patch seems to use that distinction. Are you saing as_object::get_member doesn't do what it documents as doing ?

Sandro Santilli <strk>
Project MemberIn charge of this item.
Mon 24 Aug 2015 10:39:32 AM UTC, comment #1:

It'd be a good idea to include the knowledge about special case and whtt the correct handling would be (for undefined vs. null) in a code comment. Also, a testcase for the whole ExternalInterface would still be great to have for all these fixes.

Sandro Santilli <strk>
Project MemberIn charge of this item.
Fri 21 Aug 2015 03:20:38 PM UTC, original submission:

See the attached `0001_return-string-from-getvariable.patch`.

This patch fixes following GetVariable() issues listed on bug #42395 comment 6:

  • Original code returns variable value using their original types, this patch makes it convert to String.
  • Original code failed to return +/-Infinity, Array/Object properly and crashed libgnashplugin, this patch makes it return these values properly, and stops plugin from crashing.

Special case of nonexistent and undefined variable:
Under Flash Player, calling `GetVariable()` from JavaScript to fetch value
from a declared variable in Flash with value `undefined`,
returns a string "undefined". If the variable is nonexistent
(not declared), it returns `null` object.

Under Gnash, `as_object.get_member()` didn't seem to make a distinction between
a nonexistent (not declared) variable and a declared variable with value
`undefined`.

With this limitation, the patch make `undefined` case behave like nonexistent
variable; returning `null` to JavaScript, as a workaround.

Extra note: The fix of crashing is more like a side effect of this patch;
libgnashplugin crashes if it was given a non-null Array/Object value,
returning value as String avoided the crash. Root cause of the crash is
remaining to be fixed, possibly as a part of ExternalInterface issue.

Gnash: 0.8.11dev (patched against git 88f86a3 19-Aug-2015) NPAPI
Browser: Iceweasel 10.0.12 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386

Nutchanon Wetchasit <nachanon>

 

Attached Files
file #34761:  0001_return-string-from-getvariable-v2.patch added by nachanon (1KiB - text/x-patch - Patch to make `GetVariable()` return String, with more documentation)
file #34759:  0001_return-string-from-getvariable-v2.patch added by nachanon (1KiB - text/x-patch - Patch to make `GetVariable()` return String, with more documentation)
file #34689:  0001_return-string-from-getvariable.patch added by nachanon (1KiB - text/x-patch - Patch to make `GetVariable()` return String regardless of original variable type)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by strk (Posted a comment)
  • -unavailable- added by nachanon (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Mon 25 Jan 2016 04:34:50 PM UTCstrkStatusReady For Test=>Done
      Open/ClosedOpen=>Closed
    Fri 28 Aug 2015 06:58:38 PM UTCstrkStatusIn Progress=>Ready For Test
    Fri 28 Aug 2015 01:16:04 PM UTCnachanonAttached File-=>Added 0001_return-string-from-getvariable-v2.patch, #34761
    Fri 28 Aug 2015 12:50:57 PM UTCnachanonAttached File-=>Added 0001_return-string-from-getvariable-v2.patch, #34759
    Mon 24 Aug 2015 10:39:32 AM UTCstrkStatusNone=>In Progress
      Assigned toNone=>strk
    Fri 21 Aug 2015 03:20:38 PM UTCnachanonAttached File-=>Added 0001_return-string-from-getvariable.patch, #34689

    Back to the top


    Powered by Savane 3.1-cleanup1