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

VERIFIED FIXED in mozilla7

Status

()

Core
Plug-ins
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Andrey Mironov, Assigned: bsmedberg)

Tracking

({regression})

unspecified
mozilla7
x86_64
Windows 7
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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

6 years ago
Would you be willing to hunt down a regression range using http://harthur.github.com/mozregression/ ?
Keywords: regression, regressionwindow-wanted
(Reporter)

Comment 2

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

6 years ago
Most likely a regression from bug 547359, then.  Thanks for doing that!
Blocks: 547359
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regressionwindow-wanted
(Assignee)

Updated

6 years ago
Assignee: nobody → benjamin
(Assignee)

Updated

6 years ago
Depends on: 654301
(Assignee)

Comment 4

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

finally got a mochitest test to reproduce consistently in our testsuite
(Assignee)

Comment 5

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

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

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?
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+
(Assignee)

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.
(Assignee)

Comment 11

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

Updated

6 years ago
Duplicate of this bug: 664682
http://hg.mozilla.org/mozilla-central/rev/68a835e72bfd
Target Milestone: --- → mozilla7
also
http://hg.mozilla.org/mozilla-central/rev/5f35f2648169
http://hg.mozilla.org/mozilla-central/rev/484ab902b1a1
Status: NEW → RESOLVED
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.
(Assignee)

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.
Status: RESOLVED → VERIFIED
(Assignee)

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.