Closed Bug 680402 Opened 8 years ago Closed 8 years ago

Hang on certain site


(Core :: Layout: Text and Fonts, defect, critical)

Windows 7
Not set





(Reporter: alice0775, Assigned: jfkthame)



(4 keywords)


(4 files)

Attached file WinDbg log
Build Identifier:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0a1) Gecko/20110818 Firefox/9.0a1 ID:20110818030747

Browser hang

Reproducible: Always

Steps to Reproduce:
1. Open Firefox with clean profile
2. Open Page ( )

Actual Results:
  Browser hang

Expected Results:
  Display page in about 3sec on 2.5GHz C2Q

Regression window(cached m-c hourly),
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101208 Firefox/4.0b8pre ID:20101208215553
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101208 Firefox/4.0b8pre ID:20101208230203

In local built(m-c repository)
built from 4861b72cc66d : hang
built from 0bd4cd152565 : fast

Triggered by:
4861b72cc66d	Jonathan Kew — bug 553981 - handle Hangul Jamo sequences and other special cases when marking clusters. r=karlt a=roc
Attached file hang testcase.html
Attached file not hang testcase.html
The pathological behavior here arises when we try to construct a single textrun that has many separate script runs within it. The code ends up calling SanitizeGlyphRuns each time it appends a new glyph run, and that iterates over all the existing runs, which means we get behavior that is of the order of O(n^2) where n is the number of distinct glyph runs in the complete textrun, or something horrible like that. The testcase here is a worst-case scenario because it has mixed scripts repeated over and over within a single string for which we construct a giant textrun.

This patch should solve the problem by just calling SanitizeGlyphRuns once at the end of textrun construction, instead of repeating it for each added piece.
Attachment #554556 - Flags: review?(jdaggett)
The tryserver built is improved performance remarkably. (I expect early check-in!)
Keywords: hang, perf, testcase
jdaggett, review ping? This is a simple fix for a bad regression....
Comment on attachment 554556 [details] [diff] [review]
patch, v1 - avoid calling SanitizeGlyphRuns repeatedly during textrun construction with multiple scripts

Looks fine, but I think you should add a comment noting the details in comment 3 so that we remember what to avoid when restructuring code in the future.
Attachment #554556 - Flags: review?(jdaggett) → review+
Pushed to inbound, with extra comments:
Whiteboard: [inbound]
Assignee: nobody → jfkthame
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.