patchGnash - The GNU Flash player - Patches: patch #8715, Patch for fixing callback...

 
 

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

patch #8715: Patch for fixing callback registration issue in ExternalInterface

Submitter:  Nutchanon Wetchasit <nachanon>
Submitted:  Thu 13 Aug 2015 04:25:32 PM UTC
   
 
Category:  core Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  strk Open/Closed:  Closed

Jump to the original submission

Sun 16 Aug 2015 08:56:39 AM UTC, comment #7: 

Both patches pushed:
commit:554c4638cc3a11dda8f54ba638de7a0445d345ad
commit:25395ad8ac5fa389d7cf18bbdf61daa7caa0c578

Please file a new patch for the tests, and use bug #37233 for discussion, thanks !

Sandro Santilli <strk>
Group Member
Sun 16 Aug 2015 06:10:31 AM UTC, comment #6: 

Regarding testing, tests would involve communication between browser and Gnash.
I don't think testing them solely in ActionScript would be a proper way to do.

So I might need to go through host container emulation route
to provide environment necessary for ExternalInterface testing
in sure-fire way.

Any suggestion on alternatives?

Nutchanon Wetchasit <nachanon>
Sun 16 Aug 2015 03:10:44 AM UTC, comment #5: 

Patch 2 of 2.
See the attached `0002_fix-externalinterface-callback-instance.patch`.

This patch changes following thing:

  • Original code ignored value of `instance` argument, this patch put it into proper use.


This patch used the same GC provision as previous one.

Special case of `instance=null` and `instance=undefined`:

Under Flash Player, specifying `instance` as `null` or `undefined` will make
`this` reference inside the callback return `undefined`.

There seems to be no easy way to do this under Gnash, as `invoke()`
accepts `this_ptr` as `as_object*` which has no notion of being `undefined`,
closest thing it has is `null` (represented by C NULL). Specifying it as such
will make `this` inside the callback return `null` (not `undefined`).

`as_value` of value `undefined` will also change into `null`
when it was converted to `as_object*`.

With this limitation, the patch will set `this` inside the callback to `null`
when applicable, as a workaround.

Gnash: 0.8.11dev (patched against git 6b269f9 24-May-2015) NPAPI
Browser: Iceweasel 10.0.12 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386

(file #34653)

Nutchanon Wetchasit <nachanon>
Sat 15 Aug 2015 05:19:45 PM UTC, comment #4: 

Great, thanks!
Could you also add some tests for the fixes in testsuite/actionscript.all/ExternalInterface.as ?

Sandro Santilli <strk>
Group Member
Sat 15 Aug 2015 02:31:22 AM UTC, comment #3: 

No (to both question).

Original post was written in double negative form,
mostly to mirror issues list in bug #37223 comment 4.

Sorry for the confusion.

This is the original post, rephrased and emphasised:

  • Original code used `_root.methodName`, the patch make it use user-specified `method`.
  • Original code failed with `instance=null` or `instance=undefined`, the patch make it handle these properly.
  • Original code have 3-arguments limit, the patch removed the limit.


Nutchanon Wetchasit <nachanon>
Fri 14 Aug 2015 04:17:54 PM UTC, comment #2: 

Is the limit of 3 arguments a valid one ?
Should the number of arguments to be passed to the callback limited at all ?

Sandro Santilli <strk>
Group Member
Fri 14 Aug 2015 04:14:37 AM UTC, comment #1: 

In last post I said `std::vector` was used, but I actually meant to say `std::map`. I mistyped, sorry.

Nutchanon Wetchasit <nachanon>
Thu 13 Aug 2015 04:25:32 PM UTC, original submission:  

Patch 1 of 2.
See the attached `0001_fix-externalinterface-callback-registration.patch`.

This patch fixes following ExternalInterface issues listed on bug #37223:

  • `method` argument was ignored; Gnash called `_root.methodName` instead.
  • Registration failed silently if `instance` argument is `null` or `undefined`.
  • Gnash only passed maximum 3 arguments to the callback; specifiying more than 3 would have same result as passing 0 argument.


I also took a liberty of adding Doxygen documentation on the newly-updated
`movie_root::addExternalCallback()` too.

This patch might need a review before I submit the second one,
as I didn't know much about Gnash's internal GC mechanism.
(I just used `as_object.setReachable()` after storing Function in
`std::vector`, and did nothing when the callback got replaced in `std::vector`)

Gnash: 0.8.11dev (patched against git 6b269f9 24-May-2015) NPAPI
Browser: Iceweasel 10.0.12 (debian)
System: Debian GNU/Linux 7.0 Wheezy i386

Nutchanon Wetchasit <nachanon>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files
file #34653:  0002_fix-externalinterface-callback-instance.patch added by nachanon (5KiB - application/octet-stream - Patch for fixing ExternalInterface callback's "this" instance)
file #34635:  0001_fix-externalinterface-callback-registration.patch added by nachanon (5KiB - text/x-patch - Patch for fixing ExternalInterface callback registration)

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2015-08-16 strk StatusNone Done
        Assigned toNone strk
        Open/ClosedOpen Closed
    2015-08-16 nachanon Attached File- Added 0002_fix-externalinterface-callback-instance.patch, #34653
    2015-08-13 nachanon Attached File- Added 0001_fix-externalinterface-callback-registration.patch, #34635

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code