655682 - rendering is delayed until page is completely loaded

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Stefan]

User-Agent:       
Build Identifier: Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110509 Firefox/6.0a1

The script performs six sleep calls (each for 2 s) in order to simulate DB queries. FF >= 4 does not present content until the page has been fully loaded.

Reproducible: Always

Steps to Reproduce:
1. open url
2. wait

Actual Results:  
It takes twelve seconds until content is displayed.

Expected Results:  
Start displaying as soon as possible (like FF 3.5 does).

Comment 1

[John P Baker]

Toggling the html5 parser changes behaviour.

Comment 2

[Thomas Ahlblom]

Reproduced.
Mozilla/5.0 (X11; Linux x86_64; rv:6.0a1) Gecko/20110508 Firefox/6.0a1
Mozilla/5.0 (Windows NT 6.0; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Last good nightly: 2010-05-03 First bad nightly: 2010-05-04

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=83c887dff0da&tochange=d6bb0f9e9519

And as already said, using about:config and setting html5.parser.enable to false makes even the latest nightly to use the old behavior, so i guess it's bug 373864, http://hg.mozilla.org/mozilla-central/rev/358113b3642e, that makes the difference.

Comment 3

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

The page lacks a character encoding declaration.

Comment 4

[Stefan]

UTF-8 and iso-8859-1 versions take nine seconds each:

http://www.vogtner.de/mozilla/655674/out8.php
http://www.vogtner.de/mozilla/655674/out1.php

Comment 5

[Boris Zbarsky [:bzbarsky]]

Henri, can you please have a look at this?

Comment 6

[Thomas Ahlblom]

(In reply to comment #4)
> UTF-8 and iso-8859-1 versions take nine seconds each:
> 
> http://www.vogtner.de/mozilla/655674/out8.php
> http://www.vogtner.de/mozilla/655674/out1.php

http://validator.w3.org/ shows approx 1000 errors messages each with those two files until it gives up with "Too many messages". The messages are of type "Saw U+0000 in stream."

http://www.validome.org/ gives up after 100 errors of type "Not allowed character. ASCII-CODE ' 0 '"

Maybe those problems slow down the parser or make it still use non-optimal parsing.

Comment 7

[Stefan]

The new versions are:

http://www.vogtner.de/mozilla/655674/o1.php
http://www.vogtner.de/mozilla/655674/o8.php

"This document was successfully checked as HTML5!"

Comment 9

[Thomas Ahlblom]

(In reply to comment #7)
> http://www.vogtner.de/mozilla/655674/o1.php
> http://www.vogtner.de/mozilla/655674/o8.php

Yes, they validate as HTML5, but still takes 12 s each to render. :(

Comment 10

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

Does the PHP script sleep between table rows or between tables?

Comment 11

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

Not between tables, it seems. This is probably about not doing discretionary flushes at some points in tables in order to avoid non-deterministic behavior.

Comment 12

[Stefan]

(In reply to comment #9)
> Yes, they validate as HTML5, but still takes 12 s each to render. :(

That's the reason why I have reported the issue ;-)

Comment 13

[Stefan]

(In reply to comment #10)
> Does the PHP script sleep between table rows or between tables?

It sleeps (simulates running DB query) after <TH>.

Comment 14

[Stefan]

Attachment: PHP script

Comment 15

[Boris Zbarsky [:bzbarsky]]

Henri, we used to avoid flushing inside tables because layout didn't handle it right, but I think that should all work correctly now...  So the only reason to avoid flushing in tables would be performance.

Comment 16

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

(In reply to comment #14)
> Created attachment 531372 [details]
> PHP script

I bet you'll get incremental rendering if you change
<th>Xxxx</th>
<% sleep (2); %>
to
<th>Xxxx</th><% sleep (2); %>

(In reply to comment #15)
> Henri, we used to avoid flushing inside tables because layout didn't handle
> it right, but I think that should all work correctly now...  So the only
> reason to avoid flushing in tables would be performance.

There's a new reason: Can't flush text in a foster-parenting parent without making the DOM shape flushing-dependent.

This is fixable. It could flush everything but the pending text buffer.

Comment 17

[Stefan]

> I bet you'll get incremental rendering if you change
> <th>Xxxx</th>
> <% sleep (2); %>
> to
> <th>Xxxx</th><% sleep (2); %>

Yes, indeed! http://www.vogtner.de/mozilla/655674/o8sameline.php

Comment 18

[Karl Riedling]

Amazing! This also seems to solve the problem I presented in Bug 655760 (which has been marked as a duplicate of this bug): With

<?php echo "<tr><td>.....</td></tr>\n"; flush(); sleep(3); ?>

the page is only displayed when completed. With

<?php echo "\n<tr><td>.....</td></tr>"; flush(); sleep(3); ?>

(i.e., a line feed at the beginning rather than the end of the line) the table rows are displayed incrementally!

You can compare my two test scripts:

http://www.isas.tuwien.ac.at/riedling/test/FF4_table_test.php
with LF at the end of the line, and

http://www.isas.tuwien.ac.at/riedling/test/FF4_table_test_shifted_lf.php
with LF at the beginning of the line.

Didn't somebody say line feeds do not matter in HTML code ;-) ?

Comment 19

[Boris Zbarsky [:bzbarsky]]

The problem is that if your script output a non-whitespace character other than '<' after the linefeed (even in a separate echo command!) then the linefeed and that character would both be moved out of the table by the parser.

Comment 20

[Karl Riedling]

(In reply to comment #19)
> The problem is that if your script output a non-whitespace character other
> than '<' after the linefeed (even in a separate echo command!) then the
> linefeed and that character would both be moved out of the table by the
> parser.

Do you mean that the parser does not dare displaying page contents because there might be (syntactically invalid) text between the table rows which ought to be displayed above the table? But nothing would guarantee that there is no such invalid content created immediately after the sleep() interval. From that point of view, the parser is nothing better off whether the sleep() happens before or after the linefeed!

Comment 21

[Boris Zbarsky [:bzbarsky]]

> But nothing would guarantee that there is no such invalid content created
> immediately after the sleep() interval.

Exactly.  The point is that if you already have a newline, and than _newline_ would need to move out of the table, then you can't flush that newline to the DOM yet.  Comment 16 really explains everything there is to explain here, including how Henri plans to fix this.

Comment 22

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

Attachment: Flush everything except the current text buffer in discretionary flushes when the current node is foster-parenting

Comment 23

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

Attachment: Inherently timer-dependent mochitest

Comment 24

[Stefan]

(In reply to comment #22)
Works for those testcases which provide a document charset and for the testcase which does not but preloads the output with NUL characters (out.php). 

The case of no charset and less than 1024 characters ist covered in Bug 647203.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 532186 [details] [diff] [review]
Flush everything except the current text buffer in discretionary flushes when the current node is foster-parenting

r=me

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 532187 [details] [diff] [review]
Inherently timer-dependent mochitest

So how does the timer dependence manifest itself?  Is the only interaction here between the 10ms timer in the test HTML and the 500ms timer in the sjs?  Or is the 500ms sjs timer racing against the discretionary notification timer too?

Comment 27

[Boris Zbarsky [:bzbarsky]]

I'd really like to figure out some way to write a non-randomorange test here... Ehsan, any ideas?

In particular, is there a way to communicate with the sjs from the test such that we could explicitly tell it once we've seen one <td> and it can send the second one?  That would make things work.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Waldo, see comment 27?

Comment 29

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

(In reply to comment #26)
> Or is the 500ms sjs timer racing against the discretionary notification
> timer too?

It is.

(In reply to comment #27)
> In particular, is there a way to communicate with the sjs from the test such
> that we could explicitly tell it once we've seen one <td> and it can send
> the second one?  That would make things work.

setState/getState should work here. Shame on me for not remembering that they could be used.

Comment 30

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

Fix landed for the Firefox 6 train:
http://hg.mozilla.org/mozilla-central/rev/d22583a11f15

I'll revise the test case.

Comment 31

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

Attachment: Non-racy test case

About the polling in the .sjs: I tried to store the old response object across requests in order to avoid polling, but it didn't work, so it was easier to store a string and poll than to debug the HTTP server.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 533598 [details] [diff] [review]
Non-racy test case

r=me, looks great.

Comment 33

[Jeff Walden [:Waldo]]

(In reply to comment #31)
> About the polling in the .sjs: I tried to store the old response object
> across requests in order to avoid polling, but it didn't work,

Did the object have a QueryInterface method on it?  The server specifically requires that to guarantee the server is functional as an XPCOM component (and not just as an inline script or whatever).

Comment 34

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

> r=me, looks great.

Thanks.

Test landed:
http://hg.mozilla.org/mozilla-central/rev/9bc68c12b4cc

(In reply to comment #33)
> (In reply to comment #31)
> > About the polling in the .sjs: I tried to store the old response object
> > across requests in order to avoid polling, but it didn't work,
> 
> Did the object have a QueryInterface method on it?  The server specifically
> requires that to guarantee the server is functional as an XPCOM component
> (and not just as an inline script or whatever).

The object was the nsIHttpResponse object. I assumed it had QI (since it is named nsI*) but I didn't verify.

Comment 35

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

Test case backed out due to permaorange in another mochitest. Apparently this test leaves around some state the bothers a subsequent test.

Comment 36

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

Oh, and the error was:
ERROR TEST-UNEXPECTED-FAIL | /tests/parser/htmlparser/tests/mochitest/test_compatmode.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - Error loading script at http://mochi.test:8888/tests/parser/htmlparser/tests/mochitest/file_bug655682.sjs?trigger=1:1

Comment 37

[(no longer active)]

Comment on attachment 533598 [details] [diff] [review]
Non-racy test case

Sorry for my delay here.  I think you're using setState incorrectly here.  setState takes a path as its first parameter.  The way you're using it, it throws after invocation, I think: <http://mxr.mozilla.org/mozilla-central/source/netwerk/test/httpserver/httpd.js#2781>.  I'm not sure how this test passes for you.  :-)

Comment 38

[Boris Zbarsky [:bzbarsky]]

Ehsan, the setState being called here is this one:

2589         s.importFunction(function setState(k, v)
2590         {
2591           self._setState(path, k, v);
2592         });

from httpd.js.  That will call the _setState you linked to with the right arguments.

The reason this caused problems is that this test calls SimpleTest.finish() before onload fires for this test, and in particular before that trigger script has loaded....

I think changing the test to wait for both 2 tds _and_ onload should do the trick.  Might also need to fix the script response to be a valid script.

Comment 39

[Boris Zbarsky [:bzbarsky]]

I made those changes, noted to myself that the script is in fact valid, and pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=288c2acc1e17

Looking green so far.

Comment 40

[Boris Zbarsky [:bzbarsky]]

I pushed the updated test as http://hg.mozilla.org/integration/mozilla-inbound/rev/aaff2caf8428

Comment 41

[Mounir Lamouri (:mounir)]

Test in m-c:
http://hg.mozilla.org/mozilla-central/rev/aaff2caf8428

Comment 42

[George Carstoiu]

 Mozilla/5.0 (X11; Linux i686; rv:6.0) Gecko/20100101 Firefox/6.0

Verified issue on WinXP, Ubuntu 11.04, Win7 x86, Mac OS X 10.6 using the steps from comment 0.

Page displays content as soon as the browser receives the information -> setting status to Verified Fixed.

Comment 43

[(no longer active)]

Yep, the confusion was on my part.  Sorry about that!

Comment 44

[Karl Riedling]

The bug appears to have re-surfaced in a new out-of-the-box install of Firefox 18:

In 2011, the procedure described in Comment 18 (avoiding any whitespace after a </tr> tag) solved the issue on the installation of FF4 under Win XP I had at that time, and Firefox has continued to work nicely on that old machine up to now under Firefox 19, actually rendering _both_ demo files as expected, line by line I had specified in Comment 18:

http://www.isas.tuwien.ac.at/riedling/test/FF4_table_test.php
with LF at the end of the table row line, and

http://www.isas.tuwien.ac.at/riedling/test/FF4_table_test_shifted_lf.php
with LF at the beginning of the table row line.

I have set up a new Win 7 64bit machine now, with Firefox 18 newly installed (and meanwhile updated to 19), and _neither_ of the two test scripts works as intended: Instead of displaying the tables and table rows, which are generated in intervals of 2 to 3 seconds, one by one, the page is displayed only after about 30 seconds when the script has completed. I suppose that there is something in the browser configuration that renders a browser continually upgraded from version 3.5 to 19 properly operational, while it is missing in a newly installed browser and hence causes problems with that browser.

Comment 45

[Karl Riedling]

(In reply to Karl Riedling from comment #44)

Ouups, I am so sorry: The real culprit turned out to be Avira Browser Protection. Apparently, Avira waits until it has received a full HTML page and only then forwards it to the browser if Browser Protection is on. (In fact, the problem was not only with Firefox but also with other browsers, and it was not with pages originating from localhost.) So everything is fine with Firefox! Sorry for the confusion!

Comment 46

[Boris Zbarsky [:bzbarsky]]

> Apparently, Avira waits until it has received a full HTML page and only then forwards it
> to the browser if Browser Protection is on. 

Yikes.  That's incredibly user-hostile.  :(

Comment 47

[Henri Sivonen (:hsivonen) (temporarily not reading bugmail)]

Filed bug 844714.