Closed Bug 621630 Opened 10 years ago Closed 10 years ago

Plugin not loaded when inline code tries to access it, if in a form


(Core :: Plug-ins, defect)

Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: cork, Assigned: tnikkel)




(Keywords: regression, testcase, Whiteboard: [parity-webkit] [parity-opera][softblocker])


(4 files)

Attached file testcase 1
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 (
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

I donno enough about plugins to tell if this is a bug or just something that happens to break pages expecting wrong things.
Attached file testcase 2
Keywords: testcase
Whiteboard: [parity-webkit] [parity-opera]
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:

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");
            return false;
Whiteboard: [parity-webkit] [parity-opera] → [parity-webkit] [parity-opera] softblocker
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? 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. is a test case that uses Flash Player instead of the 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.

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?
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?
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?
Switching that flush to a layout flush passed try server for what it's worth.
We used to never flush here until which made us always flush content and notify.

Then made one GetFrame call (a different one) also flush layout.

The GetFrame call in TryInstantiate hasn't changed otherwise.
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
> 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.
Attached file stack
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.
Blocks: lazyfc
No longer blocks: 558302
Aha.  Yes, EnsureInstantiation should probably Flush_Frames.
Attached patch patchSplinter Review
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 on attachment 504375 [details] [diff] [review]

Attachment #504375 - Flags: review?(bzbarsky) → review+
Closed: 10 years ago
Resolution: --- → FIXED
Verified fixed in:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b10pre) Gecko/20110121 Firefox/4.0b10pre
Component: Plug-ins → HTML: Parser
OS: All → Linux
Hardware: All → x86_64
Target Milestone: --- → flash10
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: nobody → tnikkel
You need to log in before you can comment on or make changes to this bug.