Consider enabling lazy frame construction for editable regions

NEW
Unassigned

Status

()

Core
Layout
a month ago
10 days ago

People

(Reporter: Ehsan, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qf:p1])

Historically we have disabled lazy frame construction for editable regions.  I don't remember any reasons why this is currently needed, and neither does bz.

needinfo-ing a few other people who may be aware of anything that may depend on eager frame construction for editable regions?  If there is no more dependencies on this, removing this would enable some nice optimizations by removing frame construction from some code paths that JS can trigger, such as bug 1346723.
Flags: needinfo?(mats)
Flags: needinfo?(dholbert)
Flags: needinfo?(dbaron)
(Sorry, I don't have any extra info/context on this.)
Flags: needinfo?(dholbert)
I don't remember anything here.  Perhaps hg annotate / git blame does?
Flags: needinfo?(dbaron)
I disabled it when originally implementing lazy frame construction because of failures in some tests. At the time it did not look easy to fix them, but I didn't spend a lot of time looking into it.
Still tests failing

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c13a8e84208412d7f43d8d0df095db3ad2496472&group_state=expanded
(In reply to Timothy Nikkel (:tnikkel) from comment #3)
> I disabled it when originally implementing lazy frame construction because
> of failures in some tests. At the time it did not look easy to fix them, but
> I didn't spend a lot of time looking into it.

Ah yes, makes sense.  These are places where the editor code needs to be flushing frames, but it isn't.

The good news is that now with rr it should be fairly easy to go back from the failure to the place the frame tree was accessed and that's probably around the place where a flush is needed.  (Maybe GetPrimaryFrame() would be a good place to break on.)

I think this would be a really valuable optimization to do for QF.  Do you have cycles to take it, Timothy?
Blocks: 1337841
Flags: needinfo?(tnikkel)
(Sorry, I don't have any extra info/context on this.)
Flags: needinfo?(mats)
Whiteboard: [qf:p1]
Note that for purposes of bug 1346723 this may not be enough, because we also disable lazy frame construction for native anon stuff, which text editor is.
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #7)
> Note that for purposes of bug 1346723 this may not be enough, because we
> also disable lazy frame construction for native anon stuff, which text
> editor is.

Definitely not enough. Enabling lazy frame construction for editable content doesn't change the numbers for the testcase in that bug at all.
Flags: needinfo?(tnikkel)
Right, but I also suspect there is no good reason for that restriction either.

But this bug matters much more for the performance of web mail editors, blog editors and the like.
No longer blocks: 1346723
Could you describe a bit more what needs to be done to track down the test failures?  I understand the problem is some places need to flush frames but don't.  How would I go about finding these places, and what do I do to make them flush frames?  What sort of things would require frames to be flushed?
Flags: needinfo?(ehsan)
Basically you just apply a patch like this

https://hg.mozilla.org/try/rev/5d903da115ab4590ab5c25fa7796a0c957c80954

and track down the root cause of one of these test failures

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c13a8e84208412d7f43d8d0df095db3ad2496472

Flushing frames means calling FlushPendingNotifications on the document or presshell with a FlushType of Frames or greater. This makes sure that the layout model of frames is up to do with the content model of dom nodes. Things that would require a flush would be anything that tries to access a frame, or access any other information that is stored on frames.

Although flushing is one way to fix this, there might be cases where it's better to refactor the editor to not require up-to-date frames. In general we want to avoid flushing to improve performance.
If you add new FlushPendingNotifications calls: please note that arbitrary objects may have
been destroyed, or changed, after that call.  All objects that you touch subsequently needs
to already have a strong ref (on the stack preferably), not only in the method of the call
itself but also in all callers of that method, and callers of those methods etc, out to
the nearest JS API entry point. (or you could use a Weak reference type of some kind and
do a null-check)

FlushPendingNotifications is very dangerous to call, please avoid it if at all possible.
I've been looking at this for a while, but haven't made any actual progress yet.  Lots of trying to figure out how to get debuggers and rr to work, and waiting for compilers and rr to do their job, but not so much actual results.  I'm working only a bit more today and then a bit on Sunday, and after that I'll be working April 18-26 before my next break.  So I'll be available to look at this more on April 18 if no one has taken it before then.

(Maybe it's just me, but I find printf-and-recompile to be more useful than debuggers . . .)
So looking at the tests/test_bug1315065.html failure, the patch causes selCont->CharacterExtendForDelete() to fail in TextEditor::ExtendSelectionForDelete.  I can imagine that this would require accurate frames available.  But putting this at the beginning of the method doesn't help:

    nsCOMPtr<nsIDocument> doc = GetDocument();
    doc->FlushPendingNotifications(FlushType::Frames);

So I don't know what to do.  Any suggestions?
The wpt failure (editing/run/backcolor.html) is because in Selection::getTableCellLocationFromRange, the call to mFrameSelection->GetCellLayout returns null.  Adding

  nsIPresShell* presShell = GetPresShell();
  if (presShell) {
    presShell->FlushPendingNotifications(FlushType::Frames);
  }

beforehand fixes the exception, but the test still fails with

  assert_equals: Wrong result returned (after color normalization) expected "rgb(0, 255, 255)" but got "rgba(0, 0, 0, 0)"

More debugging needed to figure that one out.
(I'm not saying that these FlushPendingNotifications should actually be checked in, I'm just trying to figure out some sort of fix as a starting point.)
Sorry, the fix I gave for the wpt failure actually fixes the unexpected fail entirely.  The tests still fail, but it's expected.  Still to do:

* editor/libeditor/tests/test_bug1315065.html (see comment 14)
* chrome://mochitests/content/a11y/accessible/tests/mochitest/textcaret/test_general.html
* editor/libeditor/tests/test_htmleditor_keyevent_handling.html
* editor/libeditor/tests/test_htmleditor_keyevent_handling.html: To do with events fired on tabbing when tabbing is disabled.  I've never looked at this code before, and there were no warnings printed to the console before the failure.
* accessible/tests/mochitest/textcaret/test_general.html: I don't even know what the features tested here are supposed to do.

I can look more into the test_bug1315065.html failure on Sunday if someone replies to comment 14, and the wpt failure has a fix described in comment 15.  I don't know if I'm a good person to work on the other two failures.
(In reply to Aryeh Gregor (:ayg) (working March 28-April 26) from comment #14)
> So looking at the tests/test_bug1315065.html failure, the patch causes
> selCont->CharacterExtendForDelete() to fail in
> TextEditor::ExtendSelectionForDelete.  I can imagine that this would require
> accurate frames available.  But putting this at the beginning of the method
> doesn't help:
> 
>     nsCOMPtr<nsIDocument> doc = GetDocument();
>     doc->FlushPendingNotifications(FlushType::Frames);
> 
> So I don't know what to do.  Any suggestions?

Perhaps something else is failing before this that also needs a flush? You could try flushing layout in the test before the problematic line (getBoundingClientRect will flush layout from js) to see if that fixes the problem. That's not a proper fix but gives helpful information to proceed with, and can be used as a general method to narrow down the problem in any test.
Adding the line

  p.getBoundingClientRect();

before the first failing is() in test_bug1315065.html does not fix the failure.
Looks like Timothy responded to the original needinfo...  Comment 20 basically suggests that the necessary flush needs to happen somewhere inside Gecko which isn't surprising.  From what I can read in the bug my guess is that something in InsertNode (assuming you're here <http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/editor/libeditor/tests/test_bug1315065.html#35> is dirtying layout and then not flushing later on when it implicitly expects the frame tree to be up to date...  Unfortunately it's hard to say something less vague without having debugged this a bit... :/
Flags: needinfo?(ehsan)
You need to log in before you can comment on or make changes to this bug.