Regression in parsing of XHTML with deferred scripts

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
DOM
P2
normal
VERIFIED FIXED
9 years ago
9 years ago

People

(Reporter: bz, Assigned: sicking)

Tracking

({regression, testcase, verified1.9.1})

Trunk
mozilla1.9.2a1
regression, testcase, verified1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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?
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
Created attachment 365302 [details] [diff] [review]
Patch to fix

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.
Created attachment 367360 [details] [diff] [review]
Patch v2
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)

Updated

9 years ago
Duplicate of this bug: 483469
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)
Created attachment 367567 [details] [diff] [review]
Patch v3

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

Updated

9 years ago
Keywords: checkin-needed
Checked in to m-c
http://hg.mozilla.org/mozilla-central/rev/5795f82e838f
Status: NEW → RESOLVED
Last Resolved: 9 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?
Keywords: fixed1.9.1 → testcase, verified1.9.1
Tests were in the patch
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.