1359411 - something in JS application or TinyMCE hangs Firefox completely

Status: VERIFIED FIXED

(Core :: DOM: Selection, defect)

Description

[Wolfgang Rosenauer [:wolfiR]]

I'm using a webmail system (Open-Xchange; used by quite some operators and isps out there) which recently hangs my Firefox completely in certain mail composing scenarios.
This appears to have started with FF53 and so far was reproduced only on Linux (will try Windows in a bit) but on different distributions and systems.

Steps to reproduce:
1. log into an Open-Xchange system
2. open a new mail composer in HTML mode (this includes a TinyMCE editor)
3. edit a few things in the editor
4. place the cursor somewhere in the middle of the content
5. use the keyboard to move the cursor fully up the editor via permanent keystroke

Result is that Firefox hangs completely, does not do any redraws anymore and does not react to anything but killing it.

(I can provide a test account on an affected system but do not want to share it with the whole internet.)

I have chosen Core/General as I have no idea at this moment what it might be.

Comment 1

[Wolfgang Rosenauer [:wolfiR]]

Just checked with Firefox on Windows and exactly the same issue.

Comment 2

[Cosmin Muntean [:cmuntean], Ecosystem QA]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
Firefox: 53.0, Build ID: 20170413192749

I have managed to reproduce this issue on latest Firefox (53.0) release and latest Nightly (55.0a1) build on Windows 7 x64.
Here is a screen recording with the issue: https://goo.gl/XMk3nV.
The browser also chased after I have tried to close it. Here is the Crash Report: bp-fbbe72a0-33aa-4dae-a1af-a53ba0170426

Considering the fact that I could not reprocue the issue on a older Firefox (52.0.2) version, I have performed a regression using the mozregression tools.

Here are the results:
Last good revision: 1e336b95cfa17f5f2d436090cd335dec30cdc7d9
First bad revision: 1befb337dbd7965c08b142ebdba4ed69c44fd7f9
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1e336b95cfa17f5f2d436090cd335dec30cdc7d9&tochange=1befb337dbd7965c08b142ebdba4ed69c44fd7f9

Looks like the following bug has the  changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1328030

Mats Palmgren, any thoughts on this?

Comment 3

[Mats Palmgren (inactive)]

> 5. use the keyboard to move the cursor fully up the editor via permanent keystroke

I'm not sure what "permanent keystroke" means here.  Which key(s) are you pressing exactly?

> (I can provide a test account on an affected system but do not want to share it with the whole internet.)

That would be most welcome; can you send me login details in a separate email?  Thanks.

Comment 4

[Wolfgang Rosenauer [:wolfiR]]

(In reply to Mats Palmgren (:mats) from comment #3)
> > 5. use the keyboard to move the cursor fully up the editor via permanent keystroke
> 
> I'm not sure what "permanent keystroke" means here.  Which key(s) are you
> pressing exactly?

"arrow up" - permanently. I didn't test if the very same happens also when I move line by line.

> > (I can provide a test account on an affected system but do not want to share it with the whole internet.)
> 
> That would be most welcome; can you send me login details in a separate
> email?  Thanks.

As Cosmin found out the following link https://gold.ox.io/admin-api/tryme/user/gold/assignment gives you access to a demo system which can be easily used to reproduce. If you run into issues with that one I can provide a dedicated one on a system I control.

Comment 5

[Mats Palmgren (inactive)]

Thanks, I can reproduce the problem in a Linux Nightly.  Investigating...

Comment 6

[Mats Palmgren (inactive)]

Attachment: Testcase

Comment 7

[Mats Palmgren (inactive)]

Here's what happens when typing UP in that testcase:

1. we call nsIFrame::PeekOffset with the "focus me..." text frame with
   aPos->mAmount == eSelectLine.
http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/layout/generic/nsFrame.cpp#8080

2. nsFrame::GetLineNumber sets blockFrame to the <body> frame
   thisLine = 1

Block(body)(1)@7f551f4895b0 <
  line 7f551f4d5768: count=1 state=block <
    Block(div)(0)@7f551f489e98 next=7f551f489c10 <
      line 7f551f4d56c8: count=1 state=inline <
        HTMLButtonControl(button)(0)@7f551f48a778 next=7f551f4d5630 <
          Block(button)(0)@7f551f4d5018  [sc=7f551f48ab80:-moz-button-content]<
            line 7f551f4d55e0: count=1 state=inline <
              Inline(i)(0)@7f551f4d5480 <
                Inline(_moz_generated_content_before)(-1)@7f551f4d54f8 <
                  Text(0)"X"@7f551f4d5570
                >
              >
            >
          >
        >
      >
      line 7f551f4d5718: count=1 state=block <
        Block(p)(1)@7f551f4d5630 <
        >
      >
    >
  >
  line 7f551f489c88: count=1 state=inline <
    Inline(editor)(1)@7f551f489c10 <
      Text(0)"focus me, then press the UP key"@7f551f489d80
    >
  >
>

3. call GetNextPrevLineFromeBlockFrame ...
   aBlockFrame=0x7f551f4895b0, aLineStart=1, aOutSideLimit=0
   (and aPos->mDirection == eDirPrevious)
http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/layout/generic/nsFrame.cpp#8100-8104
In GetNextPrevLineFromeBlockFrame we have:
(rr) p firstFrame
$3 = (nsIFrame *) 0x7f551f489e98
(rr) p lastFrame
$4 = (nsIFrame *) 0x7f551f4d5630
i.e. nearStoppingFrame=0x7f551f4d5630 farStoppingFrame=0x7f551f489e98

FindFrameAt => 0x7f551f489e98, which is a block, so return NS_OK on line 7590
http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/layout/generic/nsFrame.cpp#7577-7578

4. back in eSelectLine, set doneLooping = true, then false again on line 8156
   since aPos->mResultFrame is a block ('iter' is non-null)
http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/layout/generic/nsFrame.cpp#8115,8151,8156

5. set edgeCase = 1, thisLine = 0 and blockFrame = the <div>

6. call GetNextPrevLineFromeBlockFrame again, now w. the <div> and aOutSideLimit=1

7. searchingLine = 1 (i.e. search from the end since eDirPrevious)
(rr) p firstFrame
$10 = (nsIFrame *) 0x7f551f4d5630
(rr) p lastFrame
$11 = (nsIFrame *) 0x7f551f4d5630
i.e. nearStoppingFrame=0x7f551f4d5630 farStoppingFrame=0x7f551f4d5630

FindFrameAt => 0x7f551f4d5630, which is a block, so return NS_OK on line 7590

8. back in eSelectLine, set doneLooping = true, then false again on line 8156
   since aPos->mResultFrame is a block ('iter' is non-null)

9. set edgeCase = 1, thisLine = 0 and blockFrame = the <p>

10. third call to GetNextPrevLineFromeBlockFrame ...
    aBlockFrame=0x7f551f4d5630, aLineStart=0, aOutSideLimit=1

return NS_ERROR_FAILURE since searchingLine == -1 :
http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/layout/generic/nsFrame.cpp#7520,7523

11. back in eSelectLine, doneLooping = true and lastFrame = nullptr
    exit the inner loop since doneLooping = true, but continue the outer loop
    since result==NS_ERROR_FAILURE

12. nsFrame::GetLineNumber sets blockFrame = <div>, reset result, edgeCase, 
    and lastFrame.

13. fourth call to GetNextPrevLineFromeBlockFrame ...
    aBlockFrame=0x7f551f489e98, aLineStart=1, aOutSideLimit=0

14. searchingLine = 0, so:
(rr) p firstFrame
$23 = (nsIFrame *) 0x7f551f48a778
(rr) p lastFrame
$24 = (nsIFrame *) 0x7f551f4d5480
i.e. nearStoppingFrame=0x7f551f4d5480 farStoppingFrame=0x7f551f48a778

FindFrameAt => resultFrame = 0x7f551f48a778 (the <button> frame)
so now we do the NS_NewFrameTraversal(ePostOrder) step:
http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/layout/generic/nsFrame.cpp#7596,7598
  resultFrame 0x7f551f48a778 is NOT IsSelectable
  break, b/c we found farStoppingFrame

now the NS_NewFrameTraversal(eLeaf) step:
http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/layout/generic/nsFrame.cpp#7694,7696
  frameTraversal->Next() set resultFrame = 0x7f551f4d5570 (!IsSelectable)
  frameTraversal->Next() set resultFrame = 0x7f551f4d5630 (IsSelectable => found)
return NS_OK with aPos->mResultFrame = 0x7f551f4d5630 (the <p>)

15. back in eSelectLine, set doneLooping = true, then false again on line 8156
   since aPos->mResultFrame is a block ('iter' is non-null)

16. edgeCase = 1, thisLine = 0 and blockFrame = the <p>

That is, we're back at step 9 again and will loop 9..15 forever...

Comment 8

[Mats Palmgren (inactive)]

Before bug 1328030 the steps are as follows:

The steps are the same as the FAIL result above up to and including step 13, then:

Block(body)(1)@7f67533265b0 <
  line 7f6756743768: count=1 state=block <
    Block(div)(0)@7f6753326e98 next=7f6753326c10 <
      line 7f67567436c8: count=1 state=inline <
        HTMLButtonControl(button)(0)@7f6753327778 next=7f6756743630 <
          Block(button)(0)@7f6756743018 <
            line 7f67567435e0: count=1 state=inline <
              Inline(i)(0)@7f6756743480 <
                Inline(_moz_generated_content_before)(-1)@7f67567434f8 <
                  Text(0)"X"@7f6756743570
                >
              >
            >
          >
        >
      >
      line 7f6756743718: count=1 state=block <
        Block(p)(1)@7f6756743630 <
        >
      >
    >
  >
  line 7f6753326c88: count=1 state=inline <
    Inline(editor)(1)@7f6753326c10 <
      Text(0)"focus me, then press the UP key"@7f6753326d80 
    >
  >
>

14. searchingLine = 0, so:
(rr) p firstFrame
$1 = (nsIFrame *) 0x7f6753327778
(rr) p lastFrame
$2 = (nsIFrame *) 0x7f6756743570
i.e. nearStoppingFrame=0x7f6756743570 farStoppingFrame=0x7f6753327778

FindFrameAt => resultFrame = 0x7f6753327778 (the <button> frame)
so now we do the NS_NewFrameTraversal(ePostOrder) step:
  resultFrame 0x7f6753327778 is NOT IsSelectable
  break, b/c we found farStoppingFrame
now the NS_NewFrameTraversal(eLeaf) step:
  frameTraversal->Next() =>  resultFrame = 0x7f6756743570 (!IsSelectable)
  break, b/c we found nearStoppingFrame
=> return NS_ERROR_FAILURE

15. back in eSelectLine, set doneLooping = true ... exit the inner loop

16. call GetNextPrevLineFromeBlockFrame ...
    aBlockFrame=0x7f67533265b0, aLineStart=0, aOutSideLimit=0

searchingLine is zero, so return at:
http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/layout/generic/nsFrame.cpp#7520,7523

17. back in eSelectLine, set doneLooping = true ... exit the inner loop
18. call GetNextPrevLineFromeBlockFrame with <html> frame
    => also returns NS_ERROR_FAILURE

then finally return NS_ERROR_FAILURE in eSelectLine, b/c thisLine = -1

Comment 9

[Mats Palmgren (inactive)]

IOW, the crucial difference is that GetLastLeaf used to return the innermost
text node inside the <button> (the "X"), but now returns the <button>
http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/layout/generic/nsFrame.cpp#7562

Comment 10

[Mats Palmgren (inactive)]

Attachment: Wallpaper an infinite loop in nsIFrame::PeekOffset eSelectLine case involving NAC

Comment 11

[Mats Palmgren (inactive)]

Attachment: Reftest

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

Comment 12

[Mats Palmgren (inactive)]

We should probably rewrite the "case eSelectLine" and GetNextPrevLineFromeBlockFrame
code from scratch some day (bug 1329658), but I'm not going to attempt that here.
So this just wallpapers the problem for now.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8863470 [details] [diff] [review]
Wallpaper an infinite loop in nsIFrame::PeekOffset eSelectLine case involving NAC

># HG changeset patch
># User Mats Palmgren <[e-mail]>
># Parent  4a0d6d2d3e4d996bde670cac2c84471a13d964be
>Bug 1359411 - Wallpaper an infinite loop in nsIFrame::PeekOffset eSelectLine case involving NAC.  r=smaug
>
>diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp
>--- a/layout/generic/nsFrame.cpp
>+++ b/layout/generic/nsFrame.cpp
>@@ -7683,16 +7683,17 @@ nsFrame::GetNextPrevLineFromeBlockFrame(
>           break;
>         //always try previous on THAT line if that fails go the other way
>         frameTraversal->Prev();
>         resultFrame = frameTraversal->CurrentItem();
>         if (!resultFrame)
>           return NS_ERROR_FAILURE;
>       }
> 
>+      nsIFrame* farStoppingNextFrame = farStoppingFrame->GetNextSibling();
this needs some explanation.


>+        nsIFrame* tempFrame = frameTraversal->CurrentItem();
>+        // Step past any native anonymous content.
>+        while (tempFrame && tempFrame->GetContent()->IsInNativeAnonymousSubtree()){
>+          frameTraversal->Next();
>+          tempFrame = frameTraversal->CurrentItem();
>+        }
>+        if (!tempFrame || tempFrame == farStoppingNextFrame) {
>           break;
>+        }
So why does this method not get called when dealing with <textarea> or so?
Why aren't all the frames here in NAC

Comment 14

[Mats Palmgren (inactive)]

(In reply to Olli Pettay [:smaug] from comment #13)
> >+      nsIFrame* farStoppingNextFrame = farStoppingFrame->GetNextSibling();
> this needs some explanation.

It's a just a heuristic to fix this crash.  The NS_NewFrameTraversal(eLeaf) step
only occurs when the NS_NewFrameTraversal(ePostOrder) step fails to find
anything IsSelectable.  TBH, I'm not really sure if the eLeaf step is doing
really - it was added in http://searchfox.org/mozilla-central/commit/f447570414eaf7db70a195a73e59a1ccea19d671
so there's no explanation for why it's there.

> So why does this method not get called when dealing with <textarea> or so?

It does get called for <textarea> but since we only have text (leaf) frames
there GetLastLeaf simply returns the frame we started from.

> Why aren't all the frames here in NAC

They are, but I don't think we reach the NS_NewFrameTraversal(eLeaf) step
for <textarea> since all its child frames are normally IsSelectable.
Kbd navigation in a <textarea style="-moz-user-select:none"> is already broken
and this patch doesn't seem to make it worse.

Comment 15

[Mats Palmgren (inactive)]

s/crash/hang/

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8863470 [details] [diff] [review]
Wallpaper an infinite loop in nsIFrame::PeekOffset eSelectLine case involving NAC

Sorry, I'm having hard time to understand this. Why we need farStoppingNextFrame, especially when farStoppingFrame may point to quite different things in eDirPrevious and eDirNext.
Why farStoppingFrame isn't enough here, or why we in both eDirPrevious and eDirNext cases want next sibling?

Also, I really would like to see something like MOZ_ASSERT(!aBlockFrame->GetContent()->IsInNativeAnonymousSubtree()); in the new loop.

Comment 17

[Mats Palmgren (inactive)]

Attachment: Backout bug 1328030 since it caused a hang which is strictly worse than the original problem.

I'm sorry, I just don't have the cycles to dig deeper into this bug.
Let's backout bug 1328030 instead.

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

Comment 18

[Mats Palmgren (inactive)]

(Filed bug 1362873 to re-fix bug 1328030 later.)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2999dc55b60b
Backout bug 1328030 since it caused a hang which is strictly worse than the original problem.  r=smaug

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2999dc55b60b

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 8865234 [details] [diff] [review]
Backout bug 1328030 since it caused a hang which is strictly worse than the original problem.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1359411
[User impact if declined]: content process hangs in some cases when @contenteditable are used
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:no
[Needs manual test from QE? If yes, steps to reproduce]:sure, STR in comment 0
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:no
[Why is the change risky/not risky?]:backout to previous known good state
[String changes made/needed]:none

Comment 22

[Mats Palmgren (inactive)]

n-i to land the reftest...

Comment 23

[Gerry Chang [:gchang]]

Hi Cosmin,
Can you help check if this issue is fixed in the latest nightly?

Comment 24

[Cosmin Muntean [:cmuntean], Ecosystem QA]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Firefox: 55.0a1, Build ID: 20170501030204

Hi Garry,

I have retested this issue and is no longer reproducible on latest Nightly (55.0a1, Build ID: 20170501030204) build. I have tested this using the provided STR in comment 0 and also using the attached test case from comment 6. 
Tested on Windows 7 and 10 x64, Mac Os 10.12 and Ubuntu 14.04.

I can confirm that the issue was reproducible on older Nightly builds (eg: 55.0a1, 2017-05-01, Build ID: 20170501030204).

Comment 25

[Gerry Chang [:gchang]]

Comment on attachment 8865234 [details] [diff] [review]
Backout bug 1328030 since it caused a hang which is strictly worse than the original problem.

Fix a hang issue and was verified. Beta54+. Should be in 54 beta 8.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/13b604cdefa4

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f4846bc9ad
Reftest.  (test only: DONTBUILD)

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a9f4846bc9ad

Comment 29

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using Fx 55.0a1, build ID: 20170425030221, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 54.0b8 on Windows 10 x64, mac OS X 10.11.6 and Ubuntu 14.04 LTS.

Cheers!