129847 - hang when File -> Edit page on certain page

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Henrik Gemal]

if you do a File -> Edit page on:
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=29884
Mozilla will hang
20020308

Comment 1

[Roy Yokoyama]

it's an arabic page.
over to bidi developer.Joe: is this regression?

Comment 2

[Kathleen :Brade]

What I see is a bunch of assertions and then the app exits with no warning.  The
assertions include:
 ###!!! ASSERTION: redo line on totally empty line:
'aState.IsImpactedByFloater()', file nsBlockFrame.cpp, line 3664
 ###!!! ASSERTION: unconstrained height on totally empty line:
'NS_UNCONSTRAINEDSIZE != aState.mAvailSpaceRect.height', file nsBlockFrame.cpp,
line 3666

I'd guess an infinite loop without further investigation.

Comment 3

[Kathleen :Brade]

Attachment: more minimal testcase

This is a more minimal testcase than the bugzilla attachment url listed in the
url editfield.	Note:  the charset is required (I didn't try changing it to
other charsets) and the pre tags are required as is the blank line after the
character (0xC1 if I recall correctly).  I didn't try changing the character to
other character other than "x".

Comment 4

[Rui Xu]

Couldn't repro this bug with N6.2.1, seems a regression.

Comment 5

[Frank Tang]

nsbeta1+

Comment 6

[Jaime Rodriguez, Jr.]

Changing to ADT3 per ADT triage. This is a severe bug, but it will affect a
small population of Netscape users (BiDi, Editor and Arabic combon).

Comment 7

[Kathleen :Brade]

I disagree that this will affect a small segment.  The example html file has
only a <pre> tag with one character and a blank line in it.  I don't think there
is any "bi-directional" aspects in this bug.

Do we know that only one charset is affected or are many affected?  Can someone
please investigate?

Comment 8

[Frank Tang]

Impact Summery
Impact Platform: ALL
Impact language users: Arabic and Hebrew . total 6.3 M 1.125% of total internet
users
Probability of hitting the problem: MID, editing any Arabic/Hebrew document may
hit this bug
Severity if hit the problem in the worst case: hang
Way of recover after hit the problem: kill the app or reboot the machine.
Risk of the fix: unknown
Potential benefit of fix this problem: unknown

Comment 9

[Frank Tang]

in my debug build, I first hit this assertion when I try to reproduce it
TDLL! 77f662ac()
nsDebug::Assertion(const char * 0x101073e4
??_C@_0DN@NNHL@nsDependentSingleFragmentCString@, const char * 0x10107284
??_C@_0BF@NGIC@aStartPtr?5?$CG?$CG?5aEndPtr?$AA@, const char * 0x101073a8
??_C@_0DB@FHMP@?4?4?2?4?4?2dist?1include?1string?2nsDepe@, int 0x000000cb) line
291 + 13 bytes
nsDependentSingleFragmentCSubstring::Rebind(const char * 0x00000000, const char
* 0x00000000) line 203 + 37 bytes
nsDependentSingleFragmentCSubstring::nsDependentSingleFragmentCSubstring(const
char * 0x00000000, const char * 0x00000000) line 216 + 51 bytes
Substring(const char * const & 0x00000000, const char * const & 0x00000000) line
282 + 21 bytes
nsGenericDOMDataNode::GetData(nsAString & {...}) line 437 + 40 bytes
nsTextNode::GetData(nsTextNode * const 0x052901d4, nsAString & {...}) line 59 +
18 bytes
nsTextEditRules::ReplaceNewlines(nsIDOMRange * 0x04346308) line 1167
nsTextEditRules::Init(nsTextEditRules * const 0x052bf9d4, nsPlaintextEditor *
0x052ba8b8, unsigned int 0x00000000) line 165 + 17 bytes
nsHTMLEditRules::Init(nsHTMLEditRules * const 0x052bf9d4, nsPlaintextEditor *
0x052ba8b8, unsigned int 0x00000000) line 230 + 17 bytes
nsHTMLEditor::InitRules(nsHTMLEditor * const 0x052ba8b8) line 457 + 40 bytes
nsPlaintextEditor::EndEditorInit() line 247 + 15 bytes
nsAutoEditInitRulesTrigger::~nsAutoEditInitRulesTrigger() line 160 + 18 bytes
nsHTMLEditor::Init(nsHTMLEditor * const 0x052ba8b8, nsIDOMDocument * 0x042d33ec,
nsIPresShell * 0x0419e078, nsIContent * 0x00000000, nsISelectionController *
0x0419e088, unsigned int 0x00000000) line 321
nsEditorShell::InstantiateEditor(nsIDOMDocument * 0x042d33ec, nsIPresShell *
0x0419e078) line 866 + 55 bytes
nsEditorShell::DoEditorMode(nsIDocShell * 0x05205b28) line 925 + 26 bytes
nsEditorShell::PrepareDocumentForEditing(nsIDOMWindow * 0x051c5a7c, nsIURI *
0x0530c428) line 447 + 20 bytes
nsEditorShell::EndPageLoad(nsIDOMWindow * 0x051c5a7c, nsIChannel * 0x0530c6f0,
unsigned int 0x00000000) line 4744 + 27 bytes
nsEditorShell::OnStateChange(nsEditorShell * const 0x052d0df8, nsIWebProgress *
0x05206dc4, nsIRequest * 0x0530c6f0, int 0x000c0010, unsigned int 0x00000000)
line 4513
nsDocLoaderImpl::FireOnStateChange(nsIWebProgress * 0x05206dc4, nsIRequest *
0x0530c6f0, int 0x000c0010, unsigned int 0x00000000) line 1110
nsDocLoaderImpl::doStopDocumentLoad(nsIRequest * 0x0530c6f0, unsigned int
0x00000000) line 750
nsDocLoaderImpl::DocLoaderIsEmpty() line 647
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x05206db4, nsIRequest *
0x0531ebc8, nsISupports * 0x00000000, unsigned int 0x00000000) line 578
nsLoadGroup::RemoveRequest(nsLoadGroup * const 0x05206e98, nsIRequest *
0x0531ebc8, nsISupports * 0x00000000, unsigned int 0x00000000) line 526 + 44 bytes
PresShell::RemoveDummyLayoutRequest() line 6604 + 42 bytes
PresShell::DoneRemovingReflowCommands() line 6560
PresShell::ProcessReflowCommands(int 0x00000001) line 6383
ReflowEvent::HandleEvent() line 6167
HandlePLEvent(ReflowEvent * 0x0528ff60) line 6181
PL_HandleEvent(PLEvent * 0x0528ff60) line 590 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00d641a0) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x00010f0a, unsigned int 0x0000c177, unsigned int
0x00000000, long 0x00d641a0) line 1071 + 9 bytes
USER32! 77e41186()
00d641a0()


notice 0x00000000,  in nsDependentSingleFragmentCSubstring::Rebind
nsDependentSingleFragmentCSubstring::nsDependentSingleFragmentCSubstring
and
Substring(const char * const & 0x00000000, const char * const & 0x00000000)
in nsGenericDOMDataNode::GetData(nsAWritableString& aData)
the 
 const char *data = mText.Get1b(); is null

actually, what I got is
-
mText
{...}
+
m2b
0x00000000 ""
+
m1b
0x00000000 ""
	mAllBits	0x00000000
-
mState
{...}
	mInHeap	0x00000000
	mIs2b	0x00000000
	mIsBidi	0x00000000
	mLength	0x00000000

Comment 10

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 11

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 12

[Simon Montagu :smontagu]

I get the same assertion as in comment 9 if I change the character to 'A', but
in that case there is no hang.

Comment 13

[Kathleen :Brade]

is this a charset-specific bug?

Comment 14

[Simon Montagu :smontagu]

It's not charset-specific, I think it would occur anywhere where you have a
right-to-left character immediately following a line-break in a left-to-right
context. I have a fix which works, but I'm not yet sure if it's the right thing
to do.

Comment 15

[Simon Montagu :smontagu]

Attachment: Possible patch

Comment 16

[Simon Montagu :smontagu]

Attachment: Corrected version

Comment 17

[Frank Tang]

The origional code and the patch both looks very difficult to review. I suggest
we change the code to looks like the following

nsReflowStatus rs = NS_FRAME_COMPLETE;
#ifdef IBMBIDI
if(....)
 rv = xxxx;
if(....)
 rv = yyyy;
#else
if(....)
 rv = zzz;
if(....)
 rv = ooo;
#endif

Comment 18

[Kathleen :Brade]

I don't mind the #ifdef as it is; perhaps the patch would be clearer with -w and
more context?

Comment 19

[Simon Montagu :smontagu]

Attachment: Attachment 81787 with diff -u9w

Comment 20

[Frank Tang]

I think this one is fixed with the other bug. mark it fixed. (on both trunk and
branch)

Comment 21

[Simon Montagu :smontagu]

I still see this hang, except in builds with attachment 81787 [details] [diff] [review] applied :-)

Comment 22

[Simon Montagu :smontagu]

Attachment: Simpler patch

This patch is exactly the same logic, but written more simply, and also removes
the #ifdef IBMBIDI which is no longer necessary (bug 89203 comment 40).

Comment 23

[Simon Fraser [no longer active]]

If contentLength is 0, can we bail from MeasureText earlier?

Comment 24

[Simon Montagu :smontagu]

Attachment: Taking sfraser's question to its logical conclusion

If it comes to that, do we need to even enter MeasureText() if contentLength is
0?

Comment 25

[Simon Fraser [no longer active]]

We should probably ensure that MeasureText can be safely called with a
contentLength of 0, but optimize as you suggest. Is that the only place that
MeasureText is called?

Comment 26

[Simon Montagu :smontagu]

Attachment: Test inside MeasureText()

This moves the test on contentLength to the beginning of MeasureText() and
bails out when it is 0.

Comment 27

[rbs]

Comment on attachment 113036 [details] [diff] [review]
Test inside MeasureText()

sr=rbs

Comment 28

[Simon Fraser [no longer active]]

Comment on attachment 110977 [details] [diff] [review]
Simpler patch

old patch

Comment 29

[Simon Montagu :smontagu]

Fix checked in.