Closed Bug 467150 Opened 13 years ago Closed 13 years ago

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


(Core :: Layout: Block and Inline, defect, P3)






(Reporter: roc, Assigned: roc)



(Keywords: fixed1.9.0.11, fixed1.9.1)


(2 files)

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
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 approval]
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?
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]
Attached patch revised patchSplinter Review
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?
Attachment #351493 - Flags: review?(smontagu) → review+
Whiteboard: [needs review] → [needs landing]
Pushed 84cffcb0f3da to trunk.
Closed: 13 years ago13 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]
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.