Closed Bug 419716 Opened 16 years ago Closed 16 years ago

no params passed to a plugin when loading an application/xhtml+xml page over slow connection

Categories

(Core :: XML, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: vlad.alexander, Assigned: bent.mozilla)

References

()

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

We are a plugin vendor and one of our developers who is temporarily working on a slow DSL network noticed that Firefox 3 would create an instance of our plugin, then destroy it, then create another instance in place of the destroyed plugin but this time without passing values from <param> tags to the plugin.

We can reproduce this problem from multiple machines (Windows and OS X) running of this particular network but we cannot reproduce this on other, faster, networks.

Clearing the Offline Storage cache in FF3 then clicking on a link that opens the page containing the plugin seems to stop this behavior until this page is reloaded again.

This is a very serious issue for us because it makes our plugin unusable for some users.

Reproducible: Sometimes

Steps to Reproduce:
1. Install our plug-in. For Windows:
http://xstandard.com/download/NPXStandard.dll
... or for OS X:
http://xstandard.com/download/x-lite.dmg
(when prompted to reboot, simply close that windows and Quit and re-start the browser).

2. Go to this page:
http://xstandard.com/en/test-drive/pro/

3. Click on a link to open a page that loads our plugin. Notice the plugin loads with content inside of it and the message below the plugin says "Evaluation license". This is because data from the <param> tags is passed to the plugin. Click Back button or use breakcrumbs to navigate to the previous page. Then click again on the same link. The plugin will render with no content in it and the message below the editor will read "XStandard Lite ...".
Actual Results:  
An instance of our plugin is created, then destroyed, then another instance is create in it's place with data from param tags not passed to the plugin.

Expected Results:  
First instance of the plugin should not be destroyed and the other instance should not be created.
Sounds like a parser problem that I vaguely recall hearing about. mrbkap might remember the bug#.
Sorry, I haven't heard of anything even remotely like this.
We're pretty sure it's related to caching.

Here is a screen shot of what happens:
http://misc.xstandard.com/mozilla/419716/bad.gif

When the Refresh button is pressed, then the plug-in is correctly loaded. Here is a screen shot:
http://misc.xstandard.com/mozilla/419716/good.gif

Is there a way to run Firefox in debug mode where it writes to a log file?
Is this a regression from Firefox 2.x? can you reproduce with other plugins? With a simpler HTML page?

> Is there a way to run Firefox in debug mode where it writes to a log file?
You could build a debug version of Firefox, then try to get stack traces for the situations, when your plugin gets created/destroyed.

> This is a very serious issue for us because it makes our plugin unusable for
some users.

You should request blocking-1.9 then. Doubt you'll get it without digging deeper into it or making the issue easier to reproduce...
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Thank you for looking into this Nickolay.

>Is this a regression from Firefox 2.x?
Yes

>can you reproduce with other plugins?
Can't say because most plug-ins load in a different (non-standards compliant) way. For example, via JavaScript, <embed> element, etc.

>With a simpler HTML page?
Yes, see test case below.

We've narrowed the parameters to reproduce the problem. It now appears to be a parsing issue rather than a caching issue. Here is what is required to reproduce this problem:

1. Page that loads the editor is served with application/xhtml+xml MIME type.

2. Slow Internet connection.

3. Lots of data in <param> tags.

Here is a new test case:
http://misc.xstandard.com/mozilla/ff3/

In this test case, the problem is reproducible 100% of the time if you are on a slow Internet connection.

In the test case, click the "On HTML" link, the plug-in will load with data. Click on the "On XHTML" link, the plug-in will load blank.

Keywords: regression
So this is a problem for pages with XML content type only or were you also able to reproduce with text/html pages initially? (If the former, my wild guess is that this сould be due to the fix for bug 18333.)

In any case, there's one more relatively easy thing you could do to help diagnose this bug - figure out the regression range (with binary search) using the builds from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/

I would start around 20070130-31 to check if my wild guess was correct. Once you have a regression range, I think it's enough information in the bug to ask for blocking.

(I'm sorry I don't have time to do any actual work here.)
Is the new summary correct? Does the testcase in comment 5 show the same problem as described in comment 0? I.e. does the following happen in the testcase described in comment 5?

> Firefox 3 would create an instance of our
> plugin, then destroy it, then create another instance in place of the destroyed
> plugin but this time without passing values from <param> tags to the plugin.

Summary: Firefox 3, Offline Storage affects plugins → no params passed to a plugin when loading an application/xhtml+xml page over slow connection
>So this is a problem for pages with XML content type only
Yes. I did not know this at the time I filed this bug. I thought it was related to caching.

The last trunk build where this worked correctly was:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2007/02/2007-02-01-04-trunk/

This bug first appears in this trunk build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2007/02/2007-02-02-04-trunk/

>Is the new summary correct?
Yes and no. The root of the problem is not that param tags are not passed to the plug-in - that is the symptom I think. The problem starts with a instance of the plug-in being created, then destroyed, then in its place a another instance is created. And param tags are not being passed to the second instance of the plug-in.

Comment 0 is still correct except the test case in comment 5 is much easier to work with.

I would like to request that this bug be marked as blocking-1.9.

Thank you Nickolay!
Note that we have code that's supposed to protect against this happening: see the IsMonolithicContainer() checks in nsXMLContentSink.  And we don't instantiate plug-ins for <object> from content code until DoneAddingChildren() happens in any case.

Confirming, but someone needs to dig into this to figure out what's actually going on.
Status: UNCONFIRMED → NEW
Component: General → XML
Ever confirmed: true
Flags: blocking1.9?
QA Contact: general → xml
What would particularly help is a testcase that shows the problem even on fast connections, and one which ideally doesn't require the plug-in in question...
Do you by any chance have adblock installed? Or any other extensions?

When we have a scripted contentpolicy installed we will instantiate the plugin when frame for the <object> is first created (since we call content policies, which will create a wrapper, which will instantiate the plugin). And we'll then tear down and recreate the plugin once we start getting data.
(In reply to comment #9)
> Note that we have code that's supposed to protect against this happening: see
> the IsMonolithicContainer() checks in nsXMLContentSink.  And we don't
> instantiate plug-ins for <object> from content code until DoneAddingChildren()
> happens in any case.

Except that the XML content sink creates elements with aFromParser == PR_FALSE (it uses NS_NewElement -> NS_NewHTMLElement which passes PR_FALSE for aFromParser) and the HTML elements all basically ignore DoneAddingChildren if they were created with aFromParser == PR_FALSE.

Is there a reason the XML content sink didn't start passing aFromParser = PR_TRUE when it became incremental?
Uh... I can't think of a reason for that at all.  And that would certainly cause this bug.  How hard would it be to fix?
Why would it cause this bug? Sounded in comment 8 like it was at the second instantiation that we didn't pass in parameters.

I don't think it would be that hard to fix though. We could simply do nothing if we haven't started receiving data yet.
> Why would it cause this bug?

Because we'd instantiate the moment the <object> is inserted into the document via BindToTree(), without waiting for the params, etc.

> it was at the second instantiation that we didn't pass in parameters

It wasn't clear to me that we passed them in the first time.

In any case, it's clearly a bug.  We should fix it and see what happens.
>Do you by any chance have adblock installed?
No

>Or any other extensions?
No

>What would particularly help is a testcase that shows the
>problem even on fast connections
Here is a test case where the buffering on the ASP script is turned off and I put a 3 second sleep function in the middle of the one of the <param> tags:

http://misc.xstandard.com/mozilla/ff3fast/

In the above test case, click on "On XHTML" link. Wait 3 seconds. If the plug-in does load with data, then press Refresh button and wait 3 seconds.
Attached patch Something like this (obsolete) — Splinter Review
I still need to run mochitests on this and figure out how to test this bug in particular, but it doesn't seem to have broken anything too badly.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
We should fix this.

Ideal would be if we could test this, but I don't think we have infrastructure for that at the moment.

The patch does sound like the right approach though.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
>Ideal would be if we could test this, but I don't think we
>have infrastructure for that at the moment.
Do you mean a slow Internet connection? If so, please use test case in comment #16 and if you let us know which nightly build has the patch, we will test it over a slow Internet connection.
I'm on Linux, so unfortunately, I cannot install the plugin. Vlad, if you add <script>document.documentElement.offsetWidth</script> inside of the <object> but before your <param> tags can you reproduce the bug on a fast connection?

If so, then making a testcase for this bug should be not too hard, assuming we can find a plugin to use.
> Do you mean a slow Internet connection?

Jonas means infrastructure for automated testing of plug-in issues in our unit tests.  As in, not testing this fix to make sure it fixes this bug (which I'm happy to hear you'll help with), but adding a permanent automatic test to make sure this never breaks again.
Blake, I'm running that patch through mochitests right now, will report back once it's done...
Hmm.  We might be able to write a mochitest for this using <xhtml:script>, perhaps?  If the text node inside is big enough to get split, say, that would cause the script to think it's got its kids too early, right?

We might also be able to test with <xhtml:select>; not sure what that does with DoneAddingChildren.

>if you add <script>document.documentElement.offsetWidth</script>
>inside of the <object> but before your <param> tags can you
>reproduce the bug on a fast connection?
No

If you know of another plug-in that works on Linux/Windows and loads it's primary data from <param> tag, then I will try to reproduce the problem using that plug-in. I cannot use Flash plug-in to test this because it loads it's movie from the "data" attribute on the <object> element.
The patch in this bug passed mochitests. I saw one failure in an unrelated XUL testcase, but on a second pass through that test succeeded as well.
Going to take this off the FF3 blocker list - we can take a fix and/or get this in a dot release. 
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: blocking1.9+
Mike, application/xhtml+xml support in FF3 is very buggy. With this bug and Bug 368909 we cannot serve xstandard.com and xhtml.com as application/xhtml+xml. It undermines XHTML.
If people are actively running into this I think we should reconsider.
Flags: blocking1.9- → blocking1.9?
Ben: Here's what needs to be done to this patch:

Since we with this patch will be passing PR_TRUE for the aFromParser parameter, it's really important that we call DoneAddingChildren to all needed elements. Check which elements the HTMLContentSink calls DoneAddingChildren (it does so in several places) and make sure that the XMLContentSink call it on the same set of elements.

We also need to pass the correct value for aHaveNotified when calling DoneAddingChildren. Look at how the HTMLContentSink figures that out.

I also need to look in to the sele->WillCallDoneAddingChildren that blake did. Unless you want to comment on that mrbkap?
Assignee: mrbkap → bent.mozilla
Status: ASSIGNED → NEW
Marking this as blocking as I really think we're regression XHTML without it.
Flags: blocking1.9? → blocking1.9+
Thank you so much Jonas for addressing this issue before the release of FF3.
(In reply to comment #29)
> I also need to look in to the sele->WillCallDoneAddingChildren that blake did.
> Unless you want to comment on that mrbkap?

nsIScriptElement::WillCallDoneAddingChildren is a script-specific way to get around the problem that we're having with plugins in this bug. With this patch, <html:script> elements will be created with aFromParser = PR_FALSE, meaning they don't need to call WillCallDoneAddingChildren but <svg:script> elements don't have that option.
er, I meant PR_TRUE in that last comment.
Attached patch Updated patchSplinter Review
This is Blake's patch updated with Jonas' comments.
Attachment #311442 - Attachment is obsolete: true
Attachment #315663 - Flags: review?(jonas)
Attachment #311442 - Flags: review?(jonas)
Comment on attachment 315663 [details] [diff] [review]
Updated patch

And it passes all mochitests.
And it fixes the testcase from comment 16.
I can do some testing if you let me know where I can get a build with the patch.
Comment on attachment 315663 [details] [diff] [review]
Updated patch

Looking good
Attachment #315663 - Flags: superreview+
Attachment #315663 - Flags: review?(jonas)
Attachment #315663 - Flags: review+
Btw, the HaveNotifiedForContent implementation does make sense since we called PopContent before.
Comment on attachment 315663 [details] [diff] [review]
Updated patch

a1.9=beltzner
Attachment #315663 - Flags: approval1.9? → approval1.9+
I won't be able to check this in until Tuesday, so if someone else could do it before then that would be awesome.
Keywords: checkin-needed
(In reply to comment #42)
> so if someone else could do it before then that would be awesome.

Note that this is Blake's patch, not mine.
Fix checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 370245
Flags: in-testsuite?
Depends on: 396226
No longer depends on: 370245
Flags: wanted1.9.0.x+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: