Closed
Bug 653083
Opened 13 years ago
Closed 13 years ago
Call one method in javascript, but another executed in flash player
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla7
People
(Reporter: andrey.mir, Assigned: benjamin)
References
()
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
3.56 KB,
patch
|
Details | Diff | Splinter Review | |
46.13 KB,
patch
|
Details | Diff | Splinter Review |
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•13 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•13 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•13 years ago
|
||
Most likely a regression from bug 547359, then. Thanks for doing that!
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → benjamin
Assignee | ||
Comment 4•13 years ago
|
||
finally got a mochitest test to reproduce consistently in our testsuite
Assignee | ||
Comment 5•13 years ago
|
||
Assignee | ||
Comment 6•13 years ago
|
||
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 8•13 years ago
|
||
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 9•13 years ago
|
||
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•13 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•13 years ago
|
||
Attachment #538599 -
Attachment is obsolete: true
Comment 13•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/68a835e72bfd
Target Milestone: --- → mozilla7
Comment 14•13 years ago
|
||
also http://hg.mozilla.org/mozilla-central/rev/5f35f2648169 http://hg.mozilla.org/mozilla-central/rev/484ab902b1a1
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 15•13 years ago
|
||
Are you planing to release updates for FF4/FF5 for this fix?
Comment 16•13 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•13 years ago
|
||
The version where it's fixed so far is in the "target milestone" field.
Assignee | ||
Comment 18•13 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.
Comment 19•13 years ago
|
||
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•13 years ago
|
||
taxilian, this is the bug you had mentioned, I hope.
Comment 21•13 years ago
|
||
It certainly looks like it may be; I'll verify. Thanks!
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•