171124 - [FIXr]UI freezes loading huge file with unclosed <script>

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect, P2)

Description

[Nicolas Hüppelshäuser]

Since I can't put anything other than 'other' to the version field: the bug
report references to Mozilla 1.2Alpha.

When loading a huge file the user interface freezes.  The example file is the
index of the batik API documentation (batik version 1.5beta4).  This can be
downloaded from http://xml.apache.org/batik/dist.
Loading the same index from the web
(http://xml.apache.org/batik/javadoc/index.html) won't freeze the UI.

To be clear: 'index' means not the file 'index.html' but the index of the
documentation (reached by clicking 'Index' in the top line of the main frame).

Furthermore it seems to me that the UI freezes sometimes when the index is
displayed by a (not necessarily visible) tab.

Hope this information is usefull.
    Nicolas

Comment 1

[Greg K.]

Confirmed using FizzillaCFM/2002092611 on 10.1.5. I imagine this is basically a
request to give XPFE its' own thread, separate from Necko or Gecko. (Is there
already a bug for such an RFE?)

Reassigning to XP Toolkit/Widgets.

Comment 2

[Greg K.]

Related to bug 168884, perhaps?

Comment 3

[Max Kanat-Alexander]

This bug hasn't had a comment in over a year. If there's any new information,
feel free to re-open the bug.

-M

Comment 4

[timeless]

thanks for playing but no.

the problem is that our xml sink (which handles xhtml) doesn't behave the same
way as the html sink. My understanding is that the xml sink will not release
control until it finishes parsing, which is a problem for large files.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Good guess, but no cigar.  ;)

The real problem is that whoever wrote this page hasn't gotten this concept of
"escaping HTML you're using in examples" down.  As a result there is a <script>
tag in the page (with no </script>) and a <pre> tag (with no </pre>).  The
latter is not a problem, but the former is.

The issue is that when we hit a <script> we read in all the data till we hit
</script> into a single text token.  In this case, that's about 2MB of data. 
Oops.  The problem is that reading into the text token may not really come up
for air... checking on that right now.

In any case, a profile shows that CTextToken::ConsumeUntil takes about 50% of
the pageload time, so I'm pushing this over to parser.

maxka, next time the testcase or something before resolving bugs...

Comment 6

[Boris Zbarsky [:bzbarsky]]

It looks like the problem is that CTextToken::Consume calls Distance() a lot on
iterators that get further and further apart (since things don't fit in one ODA
event)... and Distance() is O(N) in distance between iterators, so Consume()
ends up being O(N^2) in the length of the data in the <script>.  Which is kinda
bad when there are 2.5MB of data in the <script>.

Patch coming up that cheats to reduce the number of calls to Distance().  The
patch speeds up the pageload on that testcase by a factor of 2, decreases the
time spent in CTextToken::Consume by a factor of 10, etc.... the only question
is whether there should be a better way in the string api to tell that the
distance between two iterators is "at least X" (which is what we need here). 
Note that determining this can be O(min(X,N)) (where N is the distance), not
O(N) like finding the distance is, if we write a function specifically to do it.

I could hack a little while loop that advances the iterator termStrLen checking
for end-of-string all along and that would give me a theoretically better
estimate, but would be slower than just using size_forward() in normal usage and
probably not any better in this case in practice.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134000 [details] [diff] [review]
Proposed patch

David, what do you think?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134000 [details] [diff] [review]
Proposed patch

Note that endPos comes from aScanner.EndReading() and is not modified
thereafter.  I _believe_ that this does in fact mean that
ltOffset.sizeForward() <= Distance(ltOffset, endPos) holds.  I suppose I could
add an assert to that effect, though (even if it'll mean that debug builds
_will_ freeze up a bit on pages like this).

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 134000 [details] [diff] [review]
Proposed patch

This check is only necessary because the code following it is broken and is
using a find when it means compare.  I think it would be better to simplify the
code.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 134000 [details] [diff] [review]
Proposed patch

Oh, never mind.  The string APIs still have major holes in them.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Heh.  Indeed... A function like "HasNMoreChars()" instead of Distance (one which
would stop after N chars or end of string, whichever comes first), would be
useful here...  Or having Distance return an object which has < > = operators
and an implicit conversion operator returning an int (and implementing < > =
efficiently)... or any of a variety of other things.  :(

Anyway, taking bug.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Patch checked in for 1.6b.