Closed Bug 478889 Opened 15 years ago Closed 15 years ago

Regression in parsing of XHTML with deferred scripts

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: bzbarsky, Assigned: sicking)

References

Details

(Keywords: regression, testcase, verified1.9.1)

Attachments

(2 files, 2 obsolete files)

It looks like an XHTML document that includes a deferred script fails to parse.  The regression range is between 2009-01-26-02 and 2009-01-27-02, which points to bug 461555.

I think we need this fixed in b3....  Or rather we don't want to be tweaking this code after b3.
Flags: blocking1.9.1?
Is it possible that we're trying to restart the xml parser at EOF and it doesn't deal with that?
Attached file Testcase
Marking P1 blocker right now, pending Jonas' input. The two questions are:

 - does this block?
 - do we need to fix for beta?
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
Yes, needs to block. No, doesn't need beta.
Priority: P1 → P2
Attached patch Patch to fix (obsolete) — Splinter Review
I'll write up a mochitest for xhtml+defer tonight.
Attachment #365302 - Flags: superreview?(peterv)
Attachment #365302 - Flags: review?(peterv)
Comment on attachment 365302 [details] [diff] [review]
Patch to fix

I wonder if it would work to make the while in nsExpatDriver::ConsumeToken be

  while (start != end || !mMadeFinalCallToExpat) {

and then do

    if (noMoreBuffers && !BlockedOrInterrupted() && mExpatBuffered == 0) {
      mMadeFinalCallToExpat = PR_TRUE;
    }

after the assignment to mExpatBuffered, without any of your other changes to that function. We can then also remove the flush variable.
(In reply to comment #6)
>     if (noMoreBuffers && !BlockedOrInterrupted() && mExpatBuffered == 0) {
>       mMadeFinalCallToExpat = PR_TRUE;
>     }
> 
> after the assignment to mExpatBuffered

or even do

     if (noMoreBuffers && mExpatBuffered == 0) {
       mMadeFinalCallToExpat = PR_TRUE;
     }

after the |if (BlockedOrInterrupted())| block.
(In reply to comment #6)
> (From update of attachment 365302 [details] [diff] [review])
> I wonder if it would work to make the while in nsExpatDriver::ConsumeToken be
> 
>   while (start != end || !mMadeFinalCallToExpat) {

This will loop forever at the first call to ConsumeToken since we'll never set mMadeFinalCallToExpat and so the second condition will always be true. I changed it to

while (start != end || (!mMadeFinalCallToExpat && mIsFinalChunk))

Though i'm wondering if we still need to enter the loop when
|mExpatBuffered > 0|. Can we get into this function with an empty buffer to consume, but still not be at the end of the stream, and so we'll want to tell expat to parse whatever it currently holds buffered?

I have a patch without the mExpatBuffered check which seems to pass unit tests fine, attaching that for review for now.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #365302 - Attachment is obsolete: true
Attachment #367360 - Flags: superreview?(peterv)
Attachment #367360 - Flags: review?(peterv)
Attachment #365302 - Flags: superreview?(peterv)
Attachment #365302 - Flags: review?(peterv)
Comment on attachment 367360 [details] [diff] [review]
Patch v2

This fails unit-tests on tryserver, though haven't been able to reproduce locally
Attachment #367360 - Flags: superreview?(peterv)
Attachment #367360 - Flags: review?(peterv)
Attached patch Patch v3Splinter Review
If I add back the |(BlockedOrInterrupted() && mExpatBuffered > 0)| part to the while-condition it passes all crash tests.
Attachment #367360 - Attachment is obsolete: true
Attachment #367567 - Flags: superreview?(peterv)
Attachment #367567 - Flags: review?(peterv)
Attachment #367567 - Attachment is patch: true
Attachment #367567 - Attachment mime type: application/octet-stream → text/plain
Blocks: 483644
Comment on attachment 367567 [details] [diff] [review]
Patch v3

>+  while (start != end || (!mMadeFinalCallToExpat && mIsFinalChunk) ||

I'd do:

  while (start != end || (mIsFinalChunk && !mMadeFinalCallToExpat) ||
Attachment #367567 - Flags: superreview?(peterv)
Attachment #367567 - Flags: superreview+
Attachment #367567 - Flags: review?(peterv)
Attachment #367567 - Flags: review+
Checked in to m-c
http://hg.mozilla.org/mozilla-central/rev/5795f82e838f
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
It's interesting. While this patch fixes the problem in bug 483644 on Windows too, I'm not able to see the parsing error when opening the testcase attached to this bug with Shiretoko on Windows. Are any special conditions necessary?
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.2a1
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b4pre) Gecko/20090308 Shiretoko/3.1b4pre

With this version of Shiretoko the testcase on this bug fails for me. Was this pushed to branch?
Verified fixed on trunk and 1.9.1 with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090711 Minefield/3.6a1pre ID:20090711031617

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1) Gecko/20090624 Firefox/3.5 ID:20090624012136
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Tests were in the patch
Flags: in-testsuite? → in-testsuite+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: