Call one method in javascript, but another executed in flash player

VERIFIED FIXED in mozilla7



6 years ago
6 years ago


(Reporter: Andrey Mironov, Assigned: bsmedberg)



Windows 7
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)




(2 attachments, 2 obsolete attachments)



6 years ago
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

6 years ago
Would you be willing to hunt down a regression range using ?
Keywords: regression, regressionwindow-wanted

Comment 2

6 years ago
Sure! Here is my results:

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


Comment 3

6 years ago
Most likely a regression from bug 547359, then.  Thanks for doing that!
Blocks: 547359
Ever confirmed: true
Keywords: regressionwindow-wanted


6 years ago
Assignee: nobody → benjamin


6 years ago
Depends on: 654301

Comment 4

6 years ago
Created attachment 535329 [details] [diff] [review]

finally got a mochitest test to reproduce consistently in our testsuite

Comment 5

6 years ago
Created attachment 538112 [details] [diff] [review]
Parent-side, needs work for child-side making-permanent, rev. 1

Comment 6

6 years ago
Created attachment 538599 [details] [diff] [review]
Deal with temporary identifiers, rev. 2
Attachment #538112 - Attachment is obsolete: true
Attachment #538599 - Flags: review?(cdleary)
Attachment #538599 - Flags: review?(bent.mozilla)
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?
Attachment #538599 - Flags: review?(bent.mozilla) → review+
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. :-)
Attachment #538599 - Flags: review?(cdleary)
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. ;-)
Attachment #538599 - Flags: review+

Comment 10

6 years ago
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

6 years ago
Created attachment 539291 [details] [diff] [review]
Final patch for commit, rev. 2.1
Attachment #538599 - Attachment is obsolete: true


6 years ago
Duplicate of this bug: 664682
Target Milestone: --- → mozilla7
Last Resolved: 6 years ago
Resolution: --- → FIXED
Are you planing to release updates for FF4/FF5 for this fix?

Comment 16

6 years ago
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

6 years ago
The version where it's fixed so far is in the "target milestone" field.

Comment 18

6 years ago
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.
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

6 years ago
taxilian, this is the bug you had mentioned, I hope.

Comment 21

6 years ago
It certainly looks like it may be; I'll verify.  Thanks!
Depends on: 705866
You need to log in before you can comment on or make changes to this bug.