Last Comment Bug 657279 - "ASSERTION: Didn't SaveReflowAndBoundingMetricsFor frame!"
: "ASSERTION: Didn't SaveReflowAndBoundingMetricsFor frame!"
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: MathML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Frédéric Wang (:fredw)
:
:
Mentors:
Depends on:
Blocks: 369230 maction 21479 744783
  Show dependency treegraph
 
Reported: 2011-05-15 22:27 PDT by Jesse Ruderman
Modified: 2012-04-29 15:44 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (378 bytes, application/xhtml+xml)
2011-05-15 22:27 PDT, Jesse Ruderman
no flags Details
stack trace (5.24 KB, text/plain)
2011-05-15 22:29 PDT, Jesse Ruderman
no flags Details
testcase2 (1.59 KB, text/html)
2011-07-17 05:24 PDT, Frédéric Wang (:fredw)
no flags Details
maction: transmit automatic data when the selected child changes (865 bytes, patch)
2011-07-17 05:26 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
546413: maction: transmit automatic data when the selected child changes - V2 (885 bytes, patch)
2012-03-11 04:56 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
crashtests (2.39 KB, patch)
2012-03-11 04:57 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review
546413: maction: transmit automatic data when the selected child changes - V3 (885 bytes, patch)
2012-04-26 09:47 PDT, Frédéric Wang (:fredw)
no flags Details | Diff | Splinter Review
Reftests (4.28 KB, patch)
2012-04-26 09:48 PDT, Frédéric Wang (:fredw)
karlt: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-05-15 22:27:46 PDT
Created attachment 532560 [details]
testcase
Comment 1 Jesse Ruderman 2011-05-15 22:29:30 PDT
Created attachment 532561 [details]
stack trace
Comment 2 Karl Tomlinson (:karlt) 2011-05-17 04:33:10 PDT
Looks like a regression from bug 21479.

The whole embellished hierarchy may change if the selected subexpression changes from/to an embellished operator.
Comment 3 Frédéric Wang (:fredw) 2011-07-17 05:24:58 PDT
Created attachment 546412 [details]
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).
Comment 4 Frédéric Wang (:fredw) 2011-07-17 05:26:37 PDT
Created attachment 546413 [details] [diff] [review]
maction: transmit automatic data when the selected child changes
Comment 5 Frédéric Wang (:fredw) 2012-03-11 04:56:40 PDT
Created attachment 604763 [details] [diff] [review]
546413: maction: transmit automatic data when the selected child changes - V2
Comment 6 Frédéric Wang (:fredw) 2012-03-11 04:57:51 PDT
Created attachment 604764 [details] [diff] [review]
crashtests
Comment 7 Frédéric Wang (:fredw) 2012-03-11 04:59:30 PDT
> There seems to be a separate bug when selected is changed on the <math/>
> element (m7 and m8).

I opened bug 734729
Comment 8 Karl Tomlinson (:karlt) 2012-03-11 20:37:26 PDT
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.
Comment 9 Karl Tomlinson (:karlt) 2012-03-11 23:20:04 PDT
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.
Comment 10 Frédéric Wang (:fredw) 2012-04-19 02:26:40 PDT
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.
Comment 11 Karl Tomlinson (:karlt) 2012-04-19 16:43:00 PDT
Yes, we can take these patches now.
The tests should help check that future refactoring leaves the code working as expected.
Comment 12 Frédéric Wang (:fredw) 2012-04-25 08:52:33 PDT
https://tbpl.mozilla.org/?tree=Try&rev=4f9fd5558ccc
Comment 13 Frédéric Wang (:fredw) 2012-04-26 03:36:10 PDT
(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
Comment 14 Frédéric Wang (:fredw) 2012-04-26 09:47:18 PDT
Created attachment 618692 [details] [diff] [review]
546413: maction: transmit automatic data when the selected child changes - V3
Comment 15 Frédéric Wang (:fredw) 2012-04-26 09:48:31 PDT
Created attachment 618696 [details] [diff] [review]
Reftests

crashtests converted to reftests

Note You need to log in before you can comment on or make changes to this bug.