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)
Core
DOM: Core & HTML
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)
177 bytes,
application/xhtml+xml
|
Details | |
7.68 KB,
patch
|
peterv
:
review+
peterv
:
superreview+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•15 years ago
|
||
Is it possible that we're trying to restart the xml parser at EOF and it doesn't deal with that?
Reporter | ||
Comment 2•15 years ago
|
||
Comment 3•15 years ago
|
||
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
Assignee | ||
Comment 5•15 years ago
|
||
I'll write up a mochitest for xhtml+defer tonight.
Attachment #365302 -
Flags: superreview?(peterv)
Attachment #365302 -
Flags: review?(peterv)
Comment 6•15 years ago
|
||
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.
Comment 7•15 years ago
|
||
(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.
Assignee | ||
Comment 8•15 years ago
|
||
(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.
Assignee | ||
Comment 9•15 years ago
|
||
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)
Assignee | ||
Comment 11•15 years ago
|
||
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)
Assignee | ||
Comment 12•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #367567 -
Attachment is patch: true
Attachment #367567 -
Attachment mime type: application/octet-stream → text/plain
Comment 13•15 years ago
|
||
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•15 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 14•15 years ago
|
||
Checked in to m-c http://hg.mozilla.org/mozilla-central/rev/5795f82e838f
Comment 15•15 years ago
|
||
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
Comment 16•15 years ago
|
||
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?
Comment 17•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5e48f15187ba
Keywords: fixed1.9.1
Comment 18•15 years ago
|
||
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
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•