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

VERIFIED FIXED

Status

()

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

People

(Reporter: Cork, Assigned: tnikkel)

Tracking

({regression, testcase})

Trunk
regression, testcase
Points:
---

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [parity-webkit] [parity-opera][softblocker], URL)

Attachments

(4 attachments)

(Reporter)

Description

7 years ago
Created attachment 499955 [details]
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 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.
(Reporter)

Comment 1

7 years ago
Created attachment 499956 [details]
testcase 2
(Reporter)

Updated

7 years ago
Keywords: testcase
Whiteboard: [parity-webkit] [parity-opera]

Comment 2

7 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?
(Reporter)

Comment 4

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

7 years ago
Whiteboard: [parity-webkit] [parity-opera] → [parity-webkit] [parity-opera] softblocker

Updated

7 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

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

7 years ago
Switching that flush to a layout flush passed try server for what it's worth.
(Assignee)

Comment 12

7 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

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

7 years ago
Created attachment 503758 [details]
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.
(Assignee)

Updated

7 years ago
Blocks: 502937
No longer blocks: 558302
Aha.  Yes, EnsureInstantiation should probably Flush_Frames.
(Assignee)

Comment 18

7 years ago
Created attachment 504375 [details] [diff] [review]
patch

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

r=me
Attachment #504375 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 20

7 years ago
http://hg.mozilla.org/mozilla-central/rev/332349550d46
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 21

7 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

7 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

7 years ago
Assignee: nobody → tnikkel
You need to log in before you can comment on or make changes to this bug.