Closed Bug 657279 Opened 13 years ago Closed 12 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
      No description provided.
Attached file stack trace
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
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) — 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 ReftestsSplinter 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.