Closed
Bug 621630
Opened 14 years ago
Closed 14 years ago
Plugin not loaded when inline code tries to access it, if in a form
Categories
(Core Graveyard :: Plug-ins, defect)
Core Graveyard
Plug-ins
Tracking
(blocking2.0 betaN+)
VERIFIED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: cork, Assigned: tnikkel)
References
()
Details
(Keywords: regression, testcase, Whiteboard: [parity-webkit] [parity-opera][softblocker])
Attachments
(4 files)
If the object tag for a plugin is inside a form, inline scripts following the tag will fail cause the plugin isn't loaded.
Reproducible: Always
Steps to Reproduce:
1. Install the plugin from http://fribid.se/ (http://fribid.se/releases/debian/fribid_0.2.2-1~ppa_amd64.deb)
2. Load the first testcase
3. Load the second testcase
Actual Results:
The first testcase where the object tag is outside the form element works.
The second testcase where the object tag is INSIDE the form throws a warning "TypeError: document.getElementById("plugin").SetParam is not a function"
Expected Results:
Both testcases should write "Works"
Works in chromium, opera and firefox 3.6
Behavior changed between http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fc3befa99ab7&tochange=981132d2f5d6
I donno enough about plugins to tell if this is a bug or just something that happens to break pages expecting wrong things.
Comment 2•14 years ago
|
||
Probably from bug 558302, but anyway an unintended regression.
Assignee: nobody → hsivonen
Blocks: 558302
blocking2.0: --- → betaN+
Component: Plug-ins → HTML: Parser
QA Contact: plugins → parser
How does SetParam come to existence when it does come into existence? Does it need the event loop to spin?
Hope this is enough information:
http://fribid.se/releases/source/fribid-0.2.2.tar.bz2:/fribid-0.2.2/plugin/npobject.c
static bool objHasMethod(NPObject *npobj, NPIdentifier ident) {
PluginObject *this = (PluginObject*)npobj;
char name[64];
if (!copyIdentifierName(ident, name, sizeof(name)))
return false;
switch (this->plugin->type) {
case PT_Version:
return !strcmp(name, "GetVersion");
case PT_Authentication:
case PT_Signer:
return !strcmp(name, "GetParam") || !strcmp(name, "SetParam") ||
!strcmp(name, "PerformAction") || !strcmp(name, "GetLastError");
default:
return false;
}
}
Updated•14 years ago
|
Whiteboard: [parity-webkit] [parity-opera] → [parity-webkit] [parity-opera] softblocker
Updated•14 years ago
|
Whiteboard: [parity-webkit] [parity-opera] softblocker → [parity-webkit] [parity-opera][softblocker]
Has Gecko ever guaranteed synchronous loading of plug-ins when the type attribute is present on object and, therefore, the plug-in that will get loaded is known upon the insertion of the element into the tree?
http://code.google.com/p/swfobject/source/browse/trunk/swfobject/src/swfobject.js?r=409#201 makes it look like plug-in loading in a scenario like this is expected to be timing-dependent.
It seems to me that the test cases here poke at racy behavior and the race finishes differently in the old parser and in the new parser.
http://software.hixie.ch/utilities/js/live-dom-viewer/saved/769 is a test case that uses Flash Player instead of the http://fribid.se/ plug-in.
FWIW, flushing notifications right after </object> doesn't help here. (I didn't test breaking out of the update batch.)
It seems to me that this isn't an old parser vs. the new parser thing but a Firefox 3.6 vs. 4 thing.
Debugging this shows that the plug-in isn't instantiated because the object element doesn't have a frame when nsObjectLoadingContent::TryInstantiate is called.
http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/content/base/src/nsObjectLoadingContent.cpp#1825
I am guessing that trying to fix this in the parser would be the wrong thing to do.
bz, tn, Any advice on what the right way to proceed is?
Assignee | ||
Comment 9•14 years ago
|
||
Sounds like a regression from lazy frame construction.
The content flush in nsObjectLoadingContent::TryInstantiate used to construct frames before lazy frame construction. Can we just change that flush to a layout flush?
Comment 10•14 years ago
|
||
Hmm. It'd have to be a layout flush because of the FIRST_REFLOW check? I wonder why the frame had already seen a reflow in 3.6... Or did the frame already have a plugin instance?
Can you look up the blame for why this is a eFlushContent? If there's a good performance reason, and the behavior with just the content flush used to be correct (not obvious why, with that FIRST_REFLOW thing), then perhaps we should add a flush level between Flush_ContentAndNotify and Flush_Style that would call CreateNeededFrames but not ProcessPendingRestyles?
Assignee | ||
Comment 11•14 years ago
|
||
Switching that flush to a layout flush passed try server for what it's worth.
Assignee | ||
Comment 12•14 years ago
|
||
We used to never flush here until http://hg.mozilla.org/mozilla-central/rev/384c2f37c53a which made us always flush content and notify.
Then http://hg.mozilla.org/mozilla-central/rev/81977eb555a1 made one GetFrame call (a different one) also flush layout.
The GetFrame call in TryInstantiate hasn't changed otherwise.
Assignee | ||
Comment 13•14 years ago
|
||
I didn't really do any thinking about flushing layout vs flushing something less (but more than content and notify), so don't read anything into that part of what I said.
Is there a reason why plug-ins aren't instantiated without a frame and then be told to resize and paint only once the frame exists?
Moving over to Plug-ins, since it seems non-controversial at this point that this isn't a parser bug.
Assignee: hsivonen → nobody
Component: HTML: Parser → Plug-ins
OS: Linux → All
QA Contact: parser → plugins
Hardware: x86_64 → All
Comment 15•14 years ago
|
||
> Is there a reason why plug-ins aren't instantiated without a frame
Bug 90268. Mostly historical...
OK, so the flush was added here as a safety measure, not because we really wanted to make sure to have frames. That sort of agrees with what I seem to recall. For the normal case of trying to instantiate just because we got inserted in the document, we don't want to flush frames here.
What's the callstack to that TryInstantiate call? I think we should be flushing frames higher up that callstack, at whatever place it is that knows it needs a plugin instance right now to handle the JS call.
Assignee | ||
Comment 16•14 years ago
|
||
TryInstantiate has no frame for me even in the working case. The difference happens in EnsureInstantiation. This is the stack (same in both failing and working case).
When EnsureInstantiation finds that there is no frame it asks the presshell to RecreateFramesFor the plugin's content. With lazy frame construction in the failing testcase this fails to create the frame because the plugin's parent element is the form element and it has no frame yet. It then goes on to check if the frame has the first reflow bit and flushes layout if it does.
Assignee | ||
Updated•14 years ago
|
Comment 17•14 years ago
|
||
Aha. Yes, EnsureInstantiation should probably Flush_Frames.
Assignee | ||
Comment 18•14 years ago
|
||
So this code has been mostly unchanged since bug 315841.
Since we don't have a frame, the RecreateFramesFor call can only either do the work of something already pending, or fail to create a frame. So I just replaced it with a flush of frames.
Assignee: nobody → tnikkel
Attachment #504375 -
Flags: review?(bzbarsky)
Comment 19•14 years ago
|
||
Comment on attachment 504375 [details] [diff] [review]
patch
r=me
Attachment #504375 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 20•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 21•14 years ago
|
||
Verified fixed in:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b10pre) Gecko/20110121 Firefox/4.0b10pre
Status: RESOLVED → VERIFIED
Component: Plug-ins → HTML: Parser
OS: All → Linux
Hardware: All → x86_64
Target Milestone: --- → flash10
Reporter | ||
Comment 22•14 years ago
|
||
Gah sorry for the mixed up settings
Assignee: tnikkel → nobody
Component: HTML: Parser → Plug-ins
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: flash10 → ---
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → tnikkel
Updated•3 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•