Closed Bug 1309195 Opened 3 years ago Closed 3 years ago

Mark strBuf as empty after the contents have been used or ignored

Categories

(Core :: HTML: Parser, defect, P3, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox50 - wontfix
firefox51 - wontfix
firefox52 + fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Attachments

(2 files, 1 obsolete file)

Currently, the HTML parser marks strBuf as empty by setting strBufLen to 0 when the parser starts accumulating a new sequence of characters there.

To minimize memory usage when cloning the tokenizer state and to minimize the chance of bugs, we should mark the buffer empty more eagerly: when the contents have been consumed or knowingly ignored.
Priority: -- → P3
Severity: normal → critical
Keywords: crash
Attached patch Gecko patchSplinter Review
Attached patch Java patch (obsolete) — Splinter Review
Attachment #8801004 - Attachment is obsolete: true
Attachment #8801004 - Attachment is obsolete: false
Comment on attachment 8801004 [details] [diff] [review]
Gecko patch

The difference with this patch and the one you've seen before is that upon finding a duplicate attribute, this patch explicitly discards the attribute value.
Attachment #8801004 - Flags: review?(wchen)
Attachment #8801005 - Flags: review?(wchen)
[Tracking Requested - why for this release]:

Note,: topsite tests run into this on http://www.wowtv.co.kr with breakpad id https://crash-stats.mozilla.com/report/index/8cff7edd-3102-4edb-9a87-fa9292161017
Tracking 52+ since it affects top sites.
(In reply to Carsten Book [:Tomcat] from comment #6)
> [Tracking Requested - why for this release]:
> 
> Note,: topsite tests run into this on http://www.wowtv.co.kr with breakpad
> id
> https://crash-stats.mozilla.com/report/index/8cff7edd-3102-4edb-9a87-
> fa9292161017

Are you sure that's the right URL and crash id? The crash stack shows the tokenizer was in the plaintext state, but page at the URL does not have a <plaintext> tag.
Flags: needinfo?(cbook)
Oops. I accidentally left a debugging print in the previously-attached patch.
Attachment #8801005 - Attachment is obsolete: true
Attachment #8801005 - Flags: review?(wchen)
Attachment #8802065 - Flags: review?(wchen)
Attachment #8802065 - Flags: review?(wchen) → review+
Attachment #8801004 - Flags: review?(wchen) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0b7647c33fe028f37631c606ca5df0f63644eb
Bug 1309195 - Mark strBuf as empty after the contents have been used or ignored. r=wchen.
https://hg.mozilla.org/projects/htmlparser/rev/abe62ab2a9b69ccb3b5d8a231ec1ae11154c571d
Mozilla bug 1309195 - Mark strBuf as empty after the contents have been used or ignored. r=wchen.
Un-track for 51 as the volume of crashes is low.
https://hg.mozilla.org/mozilla-central/rev/5b0b7647c33f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
hi, do you think this can be uplifted to aurora or beta in terms of scope and risk?
Flags: needinfo?(hsivonen)
This is invasive enough that I'd rather let this ride the trains so that we have more time to see breakage if there is breakage. However, I think I misattributed a crash signature to this bug, so we shouldn't have particularly high hopes for this patch to fix the crash that was attributed to this bug number.
Crash Signature: [@ nsHtml5TreeBuilder::accumulateCharacters]
Flags: needinfo?(hsivonen)
(In reply to Henri Sivonen (:hsivonen) (not reading bugmail or doing reviews until 2016-10-27) from comment #8)
> (In reply to Carsten Book [:Tomcat] from comment #6)
> > [Tracking Requested - why for this release]:
> > 
> > Note,: topsite tests run into this on http://www.wowtv.co.kr with breakpad
> > id
> > https://crash-stats.mozilla.com/report/index/8cff7edd-3102-4edb-9a87-
> > fa9292161017
> 
> Are you sure that's the right URL and crash id? The crash stack shows the
> tokenizer was in the plaintext state, but page at the URL does not have a
> <plaintext> tag.

not sure, also does not crash anymore for me :)
Flags: needinfo?(cbook)
It was recommended by the dev to let this one ride the 51+ train, makes sense.
Mark 51 won't fix to let this ride the train.
You need to log in before you can comment on or make changes to this bug.