something in JS application or TinyMCE hangs Firefox completely

VERIFIED FIXED in Firefox 54

Status

()

--
major
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: wolfiR, Assigned: mats)

Tracking

(Blocks: 1 bug, {hang})

53 Branch
mozilla55
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 wontfix, firefox54 verified, firefox55 verified)

Details

(Whiteboard: regression, URL)

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Just checked with Firefox on Windows and exactly the same issue.
(Reporter)

Updated

2 years ago
OS: Linux → All
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?
Blocks: 1328030
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
Component: General → Selection
Flags: needinfo?(mats)
Whiteboard: regression
(Assignee)

Comment 3

2 years ago
> 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.
Flags: needinfo?(mats) → needinfo?(mozilla)
(Reporter)

Comment 4

2 years ago
(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.
Flags: needinfo?(mozilla)
(Assignee)

Comment 5

2 years ago
Thanks, I can reproduce the problem in a Linux Nightly.  Investigating...
(Assignee)

Comment 6

2 years ago
Posted file Testcase
(Assignee)

Comment 7

2 years ago
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...
Assignee: nobody → mats
(Assignee)

Comment 8

2 years ago
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
(Assignee)

Comment 9

2 years ago
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
(Assignee)

Comment 12

2 years ago
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 on attachment 8863470 [details] [diff] [review]
Wallpaper an infinite loop in nsIFrame::PeekOffset eSelectLine case involving NAC

># HG changeset patch
># User Mats Palmgren <mats@mozilla.com>
># 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
(Assignee)

Comment 14

2 years ago
(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.
(Assignee)

Comment 15

2 years ago
s/crash/hang/
Keywords: hang
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.
Attachment #8863470 - Flags: review?(bugs) → review-
Attachment #8865234 - Flags: review?(bugs) → review+
(Assignee)

Updated

2 years ago
Blocks: 1362873
(Assignee)

Comment 18

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

Comment 19

2 years ago
Pushed by mpalmgren@mozilla.com:
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

2 years ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/2999dc55b60b
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 21

2 years ago
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
Attachment #8865234 - Flags: approval-mozilla-beta?
(Assignee)

Comment 22

2 years ago
n-i to land the reftest...
Flags: needinfo?(mats)
Flags: in-testsuite?
Hi Cosmin,
Can you help check if this issue is fixed in the latest nightly?
Flags: qe-verify+
Flags: needinfo?(cosmin.muntean)
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).
status-firefox55: fixed → verified
status-firefox-esr52: --- → unaffected
Flags: needinfo?(cosmin.muntean)
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.
Attachment #8865234 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Updated

2 years ago
Flags: needinfo?(mats)
Flags: in-testsuite?
Flags: in-testsuite+
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!
Status: RESOLVED → VERIFIED
Flags: qe-verify+
status-firefox54: fixed → verified
status-firefox53: affected → wontfix
You need to log in before you can comment on or make changes to this bug.