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

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Andrey Mironov]

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 http://dl.dropbox.com/u/11878831/interop-demo/demo.xhtml
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 [:bzbarsky]]

Would you be willing to hunt down a regression range using http://harthur.github.com/mozregression/ ?

Comment 2

[Andrey Mironov]

Sure! Here is my results:

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

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e9b7e0b582
1d&tochange=e9312d05488f

Comment 3

[Boris Zbarsky [:bzbarsky]]

Most likely a regression from bug 547359, then.  Thanks for doing that!

Comment 4

[Benjamin Smedberg]

Attachment: Test

finally got a mochitest test to reproduce consistently in our testsuite

Comment 5

[Benjamin Smedberg]

Attachment: Parent-side, needs work for child-side making-permanent, rev. 1

Comment 6

[Benjamin Smedberg]

Attachment: Deal with temporary identifiers, rev. 2

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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

NS_STACKCLASS

::: 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)]

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)]

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]

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]

Attachment: Final patch for commit, rev. 2.1

Comment 13

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/68a835e72bfd

Comment 14

[Marco Bonardo [:mak]]

also
http://hg.mozilla.org/mozilla-central/rev/5f35f2648169
http://hg.mozilla.org/mozilla-central/rev/484ab902b1a1

Comment 15

[[e-mail]]

Are you planing to release updates for FF4/FF5 for this fix?

Comment 16

[Dmitry]

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 [:bzbarsky]]

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

Comment 18

[Benjamin Smedberg]

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]]

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]

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

Comment 21

[Richard Bateman]

It certainly looks like it may be; I'll verify.  Thanks!