Last Comment Bug 653083 - Call one method in javascript, but another executed in flash player
: Call one method in javascript, but another executed in flash player
: regression
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal with 2 votes (vote)
: mozilla7
Assigned To: Benjamin Smedberg [:bsmedberg]
: 664682 (view as bug list)
Depends on: 705866 654301
Blocks: 547359
  Show dependency treegraph
Reported: 2011-04-27 03:16 PDT by Andrey Mironov
Modified: 2011-11-28 13:49 PST (History)
15 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Test (3.56 KB, patch)
2011-05-26 07:19 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Splinter Review
Parent-side, needs work for child-side making-permanent, rev. 1 (23.62 KB, patch)
2011-06-08 14:10 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Splinter Review
Deal with temporary identifiers, rev. 2 (45.92 KB, patch)
2011-06-10 13:54 PDT, Benjamin Smedberg [:bsmedberg]
bent.mozilla: review+
cdleary: review+
Details | Diff | Splinter Review
Final patch for commit, rev. 2.1 (46.13 KB, patch)
2011-06-14 12:56 PDT, Benjamin Smedberg [:bsmedberg]
no flags Details | Diff | Splinter Review

Description Andrey Mironov 2011-04-27 03:16:38 PDT
User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0) Gecko/20100101 Firefox/4.0

I've discovered strange Flash Player behavior in Firefox 4. When I call flash method from javascript the incorrect function in flash executed.

In the demo application I register 40 callback functions (getValue1, setValue1, getValue2, setValue2, ... getValue20, setValue20) using ExternalInterface.addCallback. And then in javascript I call this methods. In the first argument I pass the name of the method I call.
When flash function called it compares the passed name with its own name and if they are the same write 'OK:' in log console or write 'FAIL:' if they are not. 

I was able to reproduce this bug in Firefox 4 only. I also tested in IE9 and Google Chrome and it works fine.
I will test it in FF4 on Mac later and will add the results.

Reproducible: Sometimes

Steps to Reproduce:
1. Open
2. Click "Reload" button several times until you see red message starts with "FAIL:". Sometimes it happens on the second time, sometimes on the 10-20th.
3. The first name after "FAIL:" is the name of the function called in javascript and the second is the name of the called function in flash.

Actual Results:  
"FAIL:" messages in log console after several page reloads

Expected Results:  
Always "OK:" message in log console
Comment 1 Boris Zbarsky [:bz] (TPAC) 2011-04-27 06:36:42 PDT
Would you be willing to hunt down a regression range using ?
Comment 2 Andrey Mironov 2011-04-27 12:16:23 PDT
Sure! Here is my results:

Last good nightly: 2010-03-23 First bad nightly: 2010-03-24

Comment 3 Boris Zbarsky [:bz] (TPAC) 2011-04-27 12:45:59 PDT
Most likely a regression from bug 547359, then.  Thanks for doing that!
Comment 4 Benjamin Smedberg [:bsmedberg] 2011-05-26 07:19:11 PDT
Created attachment 535329 [details] [diff] [review]

finally got a mochitest test to reproduce consistently in our testsuite
Comment 5 Benjamin Smedberg [:bsmedberg] 2011-06-08 14:10:32 PDT
Created attachment 538112 [details] [diff] [review]
Parent-side, needs work for child-side making-permanent, rev. 1
Comment 6 Benjamin Smedberg [:bsmedberg] 2011-06-10 13:54:47 PDT
Created attachment 538599 [details] [diff] [review]
Deal with temporary identifiers, rev. 2
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-13 15:59:26 PDT
Comment on attachment 538599 [details] [diff] [review]
Deal with temporary identifiers, rev. 2

Review of attachment 538599 [details] [diff] [review]:

Looks great!

::: dom/plugins/base/nsNPAPIPlugin.h
@@ +174,2 @@
>  {
> +  JSContext* cx = GetJSContext(npp);

It's possible that this could fail, right? Since this is in the parent is there any way we could handle that?

::: dom/plugins/ipc/PluginIdentifierChild.h
@@ +77,5 @@
>    }
> +  void MakePermanent();
> +
> +  class StackIdentifier


::: dom/plugins/ipc/PluginIdentifierParent.cpp
@@ +73,5 @@
> +    return false;
> +
> +  JSAutoRequest ar(cx);
> +  JSString* str = JSID_TO_STRING(id);
> +  JSString* str2 = JS_InternJSString(cx, str);

This can fail, you need to null check and return false.

@@ +93,5 @@
> +  PluginInstanceParent* inst = GetInstance(aObject);
> +  mIdentifier = inst->Module()->GetIdentifierForNPIdentifier(inst->GetNPP(), aIdentifier);
> +}
> +
> +PluginIdentifierParent::StackIdentifier::~StackIdentifier()

Nit: Can you add braces to these single-line if blocks? In a few other places too.

::: dom/plugins/ipc/PluginIdentifierParent.h
@@ +73,5 @@
> +    StackIdentifier(PluginInstanceParent* inst, NPIdentifier aIdentifier);
> +    StackIdentifier(NPObject* aObject, NPIdentifier aIdentifier);
> +    ~StackIdentifier();
> +
> +    operator PluginIdentifierParent*() {

Hm, for the child one you did:

  PluginIdentifierChild* operator->() { return mActor; }

Can you make these the same? s/mActor/mIdentifier/ and s/operator->/operator Actor*/ maybe?

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +347,4 @@
>  {
> +    if (aTemporary) {
> +        NS_ERROR("Plugins don't create temporary identifiers.");
> +        return NULL; // should abort the plugin

Nit: We've been using nsnull in this file...

::: dom/plugins/ipc/PluginModuleParent.h
@@ +154,5 @@
>  #endif
> +    ScopedRunnableMethodFactory<PluginModuleParent>& GetTaskFactory() {
> +        return mTaskFactory;
> +    }

Hm... What's this all about? Something from another patch?

::: dom/plugins/ipc/PluginScriptableObjectChild.cpp
@@ +659,5 @@
>      *aHasMethod = false;
>      return true;
>    }
> +  PluginIdentifierChild::StackIdentifier id(aId);

You don't want to use a typedef like you did in the parent files?
Comment 8 Chris Leary [:cdleary] (not checking bugmail) 2011-06-14 11:10:53 PDT
Comment on attachment 538599 [details] [diff] [review]
Deal with temporary identifiers, rev. 2

Review of attachment 538599 [details] [diff] [review]:

I don't understand the plugin actor model well enough to check GC safety in this patch, and bent already checked the API usage in his review. (Not sure that's worth a separate review to begin with.) I can waste someone's time and have them explain the plugin architecture, but I think it's more prudent to just cancel my review request. :-)
Comment 9 Chris Leary [:cdleary] (not checking bugmail) 2011-06-14 11:44:04 PDT
Comment on attachment 538599 [details] [diff] [review]
Deal with temporary identifiers, rev. 2

Review of attachment 538599 [details] [diff] [review]:

Sure, the same-string-after-successful-interning assumption is fine. (Like many other things, that may have to change when we switch to a moving GC. Except this assumption is well documented, unlike many other things. ;-)
Comment 10 Benjamin Smedberg [:bsmedberg] 2011-06-14 11:54:18 PDT
I did s/mActor/mIdentifier/ for Child::StackIdentifier, but I didn't change the ->/operator. On the parent side, the value is used as a pointer directly. On the child, we only call ->ToNPIdentifier() on it, and so they have to be different.

Removed GetTaskFactory, it was from a previous version of Enumerate which was not GC-safe.

Switched to the anonymous typedef.

I didn't switch to nsnull, because there are plenty of NULLs in that file and I've been using NULL in all new code.
Comment 11 Benjamin Smedberg [:bsmedberg] 2011-06-14 12:56:42 PDT
Created attachment 539291 [details] [diff] [review]
Final patch for commit, rev. 2.1
Comment 12 Benjamin Smedberg [:bsmedberg] 2011-06-16 08:14:50 PDT
*** Bug 664682 has been marked as a duplicate of this bug. ***
Comment 13 Marco Bonardo [::mak] 2011-06-24 02:50:33 PDT
Comment 15 2011-07-08 09:49:26 PDT
Are you planing to release updates for FF4/FF5 for this fix?
Comment 16 Dmitry 2011-07-12 07:11:37 PDT
Can you please specify the fix version of this issue, because of it's a blocker and showstopper for all "FlashPlayer <-> JS communication" related features
Comment 17 Boris Zbarsky [:bz] (TPAC) 2011-07-12 07:47:11 PDT
The version where it's fixed so far is in the "target milestone" field.
Comment 18 Benjamin Smedberg [:bsmedberg] 2011-07-19 07:59:16 PDT
Firefox 7 is the first release that will contain this fix, scheduled for release around 27-Sep. Due to the new rapid release schedule, there are not backport releases except for critical security bugs.
Comment 19 Virgil Dicu [:virgil] [QA] 2011-08-24 06:04:03 PDT
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0) Gecko/20100101 Firefox/7.0

Verified fixed in F7 beta1, using the STR from the description. The issue was no longer reproducible.
Comment 20 Benjamin Smedberg [:bsmedberg] 2011-11-01 11:37:37 PDT
taxilian, this is the bug you had mentioned, I hope.
Comment 21 Richard Bateman 2011-11-01 11:47:23 PDT
It certainly looks like it may be; I'll verify.  Thanks!

Note You need to log in before you can comment on or make changes to this bug.