nsContinuingTextFrame::Destroy doesn't need to clear the textrun if the frame is empty

RESOLVED FIXED

Status

()

P3
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

({fixed1.9.0.11, fixed1.9.1})

Trunk
fixed1.9.0.11, fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

See bug 455826. There's a reviewed patch there; the nsContinuingTextFrame::Destroy part needs to be landed.
Flags: blocking1.9.1?
Whiteboard: [needs landing]
Flags: blocking1.9.1? → wanted1.9.1+
Priority: -- → P3
Pushed to trunk, aaeb20c61fca
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
Created attachment 351037 [details] [diff] [review]
patch that landed on trunk

Performance win on bug 455826. Should be safe after it's baked on trunk a bit.
Attachment #351037 - Flags: review+
Attachment #351037 - Flags: approval1.9.1?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [needs 191 approval] → [needs landing or new patch]
I backed this out to try to fix Windows random mochitest timeouts
This was the cause of the random orange.
Whiteboard: [needs landing or new patch] → [needs new patch]
Created attachment 351493 [details] [diff] [review]
revised patch

The random crashes were because in some cases, the textrun's userdata was referring to an nsContinuingTextFrame which had been deleted, so when we came to expire the textrun after 30 seconds, we looked at a deleted frame in ClearAllTextRunReferences. Now, most of the time we got to "if (aFrame->GetTextRun() != aTextRun)" and read some garbage value in aFrame->mTextRun and aborted safely. It seems only on Windows that memory has sometimes actually been unmapped and so we hit a seg fault 30 seconds after the test that actually triggered the bug.

So this patch sets the TEXT_IN_TEXTRUN_USER_DATA flag on every text frame that is mentioned in a textrun's userdata, and we always flush a textrun if we destroy a frame that's mentioned in its userdata. Also, this patch adds an assertion to ClearAllTextRunReferences that calls GetType on the frame, which means that if the frame has been deleted we will almost certainly crash on all platforms (and if not, we will almost certainly fail the assertion).
Attachment #351493 - Flags: review?(smontagu)
Whiteboard: [needs new patch] → [needs review]
Comment on attachment 351037 [details] [diff] [review]
patch that landed on trunk

(clearing nom)
Attachment #351037 - Flags: approval1.9.1?

Updated

10 years ago
Attachment #351493 - Flags: review?(smontagu) → review+
Whiteboard: [needs review] → [needs landing]
Pushed 84cffcb0f3da to trunk.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
Comment on attachment 351493 [details] [diff] [review]
revised patch

Performance improvement related to bug 430332
Attachment #351493 - Flags: approval1.9.1?
Attachment #351493 - Flags: approval1.9.1? → approval1.9.1+
Whiteboard: [needs 191 approval] → [needs 191 landing]
Pushed e7d1975fdecb to 1.9.1
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Blocks: 489647
Depends on: 455826
I just landed "revised patch" from here on 1.9.0.x, to fix bug 489647.
Keywords: fixed1.9.0.10
Depends on: 472776
You need to log in before you can comment on or make changes to this bug.