Closed Bug 657279 Opened 14 years ago Closed 13 years ago

"ASSERTION: Didn't SaveReflowAndBoundingMetricsFor frame!"

Categories

(Core :: MathML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: jruderman, Assigned: fredw)

References

Details

(Keywords: assertion, testcase)

Attachments

(5 files, 3 obsolete files)

Attached file testcase (deleted) —
No description provided.
Attached file stack trace (deleted) —
Looks like a regression from bug 21479. The whole embellished hierarchy may change if the selected subexpression changes from/to an embellished operator.
Blocks: 21479
Attached file testcase2 (deleted) —
Testcase with various kinds of change. There seems to be a separate bug when selected is changed on the <math/> element (m7 and m8).
Blocks: maction
Assignee: nobody → fred.wang
Attachment #546413 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #604763 - Flags: review?(karlt)
Attached patch crashtests (obsolete) (deleted) — Splinter Review
Attachment #604764 - Flags: review?(karlt)
> There seems to be a separate bug when selected is changed on the <math/> > element (m7 and m8). I opened bug 734729
Comment on attachment 604764 [details] [diff] [review] crashtests Can this be turned into a reftest against a static dom to check that rendering is as expected? Reftests also fail when there are assertions. > load 557474-1.html > load 655451-1.xhtml > load 654928-1.html >+load bug-657279.html Looks like the filename convention here is a little different.
Attachment #604764 - Flags: review?(karlt) → review+
Comment on attachment 604763 [details] [diff] [review] 546413: maction: transmit automatic data when the selected child changes - V2 I assume this fixes the assertion for these testcases but I fear there will be other occurrences of the assertion. There is a general problem that when a frame changes to or from an embellished operator, the parent frame needs to recheck whether or not it is an embellished operator. Perhaps we need a method similar to TransmitAutomaticData that propagates up the hierarchy until there is no change to the NS_MATHML_EMBELLISH_OPERATOR flag (and perhaps to other info in mEmbellishData and to NS_MATHML_SPACE_LIKE). However, changing the existing TransmitAutomaticData() to do this may be inefficent if it continues to be called from RebuildAutomaticDataForChildren(), which calls TransmitAutomaticData on all children and then on their parent. I can't remember exactly what else is done in TransmitAutomaticData (in various implementations), but perhaps we should rethink what RebuildAutomaticDataForChildren() does, and instead handle changes that propagate up the hierarchy (i.e. TransmitAutomaticData calls) separately. I guess such changes would include the NS_MATHML_SPACE_LIKE flag and PresentationData::baseFrame. However there are some other things to bear in mind: Attribute changes on mstyle (and math) need to trigger this TransmitAutomaticData() in descendant frames. This is currently done through ReLayoutChildren(). Check that calling TransmitAutomaticData during Reflow() (as the maction frame does) is early enough to propagate to TransmitAutomaticData() on the parent, given the parent has begun reflow at this stage. This may be fine because Reflow of a frame typically depends on Reflow of the children. But AttributeChanged is probably a more appropriate place than BuildDisplayList, Reflow, and Place to check the "selected" attribute.
Attachment #604763 - Flags: review?(karlt) → review+
Blocks: 744783
Karl, do you think we should take these patches now (with crashtests converted to reftests) and handle the refactoring of these data transmission in another bug? I expect Andrii to improve a lot our code for dynamic changes this summer and your suggestion of comment 9 looks something that can fit in his project.
OS: Mac OS X → All
Hardware: x86_64 → All
Yes, we can take these patches now. The tests should help check that future refactoring leaves the code working as expected.
(In reply to Frédéric Wang from comment #12) > https://tbpl.mozilla.org/?tree=Try&rev=4f9fd5558ccc I forgot to handle the reftest-wait class when I converted the crashtests into reftests. https://tbpl.mozilla.org/?tree=Try&rev=1181eb8fc887
Attached patch Reftests (deleted) — Splinter Review
crashtests converted to reftests
Attachment #604763 - Attachment is obsolete: true
Attachment #604764 - Attachment is obsolete: true
Attachment #618696 - Flags: review?(karlt)
Attachment #618696 - Flags: review?(karlt) → review+
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: