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

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Cork]

Attachment: 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.

Comment 1

[Cork]

Attachment: testcase 2

Comment 2

[Benjamin Smedberg]

Probably from bug 558302, but anyway an unintended regression.

Comment 3

[Henri Sivonen (:hsivonen)]

How does SetParam come to existence when it does come into existence? Does it need the event loop to spin?

Comment 4

[Cork]

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

Comment 5

[Henri Sivonen (:hsivonen)]

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.

Comment 6

[Henri Sivonen (:hsivonen)]

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.

Comment 7

[Henri Sivonen (:hsivonen)]

FWIW, flushing notifications right after </object> doesn't help here. (I didn't test breaking out of the update batch.)

Comment 8

[Henri Sivonen (:hsivonen)]

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?

Comment 9

[Timothy Nikkel (:tnikkel)]

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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 11

[Timothy Nikkel (:tnikkel)]

Switching that flush to a layout flush passed try server for what it's worth.

Comment 12

[Timothy Nikkel (:tnikkel)]

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.

Comment 13

[Timothy Nikkel (:tnikkel)]

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.

Comment 14

[Henri Sivonen (:hsivonen)]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

Comment 16

[Timothy Nikkel (:tnikkel)]

Attachment: 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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Aha.  Yes, EnsureInstantiation should probably Flush_Frames.

Comment 18

[Timothy Nikkel (:tnikkel)]

Attachment: 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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 504375 [details] [diff] [review]
patch

r=me

Comment 20

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/332349550d46

Comment 21

[Cork]

Verified fixed in:
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b10pre) Gecko/20110121 Firefox/4.0b10pre

Comment 22

[Cork]

Gah sorry for the mixed up settings