Crash [@ nsTextEditorState::InitializeKeyboardEventListeners] on print preview close with iframe, position:fixed and input

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
Layout
--
critical
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Ehsan)

Tracking

({crash, regression, testcase})

Trunk
mozilla2.0b7
x86
Windows 7
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(crash signature)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

7 years ago
Created attachment 474196 [details]
testcase

I guess this is also a fallout from bug 534785.
The attached testcase crashes in current trunk build on print preview or when closing print preview.

http://crash-stats.mozilla.com/report/index/6bb2dd74-5ca8-4c5c-9a57-6f0f82100910
0  	xul.dll  	nsTextEditorState::InitializeKeyboardEventListeners  	 content/html/content/src/nsTextEditorState.cpp:1813
1 	xul.dll 	nsHTMLInputElement::InitializeKeyboardEventListeners 	content/html/content/src/nsHTMLInputElement.cpp:4280
2 	xul.dll 	nsTextControlFrame::SetInitialChildList 	layout/forms/nsTextControlFrame.cpp:1430
3 	xul.dll 	nsCSSFrameConstructor::ConstructFrameFromItemInternal 	layout/base/nsCSSFrameConstructor.cpp:3862
4 	xul.dll 	nsCSSFrameConstructor::ConstructFramesFromItem 	layout/base/nsCSSFrameConstructor.cpp:5452
5 	xul.dll 	nsCSSFrameConstructor::ConstructFramesFromItemList 	layout/base/nsCSSFrameConstructor.cpp:9430
6 	xul.dll 	nsCSSFrameConstructor::ProcessChildren 	layout/base/nsCSSFrameConstructor.cpp:9546
7 	xul.dll 	nsCSSFrameConstructor::ConstructBlock 	layout/base/nsCSSFrameConstructor.cpp:10596
8 	xul.dll 	nsCSSFrameConstructor::ConstructDocElementFrame 	layout/base/nsCSSFrameConstructor.cpp:2483
9 	xul.dll 	nsCSSFrameConstructor::ContentRangeInserted 	layout/base/nsCSSFrameConstructor.cpp:6848
10 	xul.dll 	nsCSSFrameConstructor::ContentInserted 	layout/base/nsCSSFrameConstructor.cpp:6747
11 	xul.dll 	PresShell::InitialReflow 	layout/base/nsPresShell.cpp:2645
etc..
I couldn't reproduce this (on a Mac).  Can you please provide the exact steps to reproduce?
(Reporter)

Comment 2

7 years ago
- Load testcase
- Click on File->Print Preview
- Close Print Preview
Mac doesn't have print preview, afaik, so you probably should try on linux or windows.
(In reply to comment #2)
> - Load testcase
> - Click on File->Print Preview
> - Close Print Preview
> Mac doesn't have print preview, afaik, so you probably should try on linux or
> windows.

Yes, but its printing code should use the same code path as Windows and Linux...  Can you reproduce this by dropping the testcase in layout/forms/crashtest and adding the reftest-print class to the HTML element?
(Reporter)

Comment 4

7 years ago
Afaik, that is setting the testcase in page-mode, according to this code:
http://mxr.mozilla.org/mozilla-central/source/layout/tools/reftest/reftest.js#831
And it can very well not crash in page mode, but still crash in print preview.
(Reporter)

Comment 5

7 years ago
Created attachment 474451 [details]
testcase in page mode

I'm not getting a crash in page mode.
(Reporter)

Comment 6

7 years ago
It turns out this regressed between 2010-07-12 and 2010-07-14:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2010-07-12+03%3A00%3A00&enddate=2010-07-14+09%3A00%3A00
During that time period, it regressed from not crashing at all, to crashing directly on print preview (in current trunk builds, it's crash on print preview close).
I guess a regression from bug 289384.
Blocks: 289384
I managed to reproduce it under Linux...
Created attachment 474760 [details] [diff] [review]
Patch (v1)

InitializeKeyboardEventListeners tries to grab the first child frame, which might not exist for the duplicate frames.  This patch makes sure that doesn't happen.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #474760 - Flags: review?(roc)
Attachment #474760 - Flags: approval2.0?
Attachment #474760 - Flags: review?(roc)
Attachment #474760 - Flags: review+
Attachment #474760 - Flags: approval2.0?
Attachment #474760 - Flags: approval2.0+
Created attachment 474884 [details] [diff] [review]
For check-in
Nominating as blocking, since this is a regression crash.
blocking2.0: --- → ?
Keywords: checkin-needed
Blocks: 534785
No longer blocks: 289384
http://hg.mozilla.org/mozilla-central/rev/d668a9be7bd4
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6
Backed out: http://hg.mozilla.org/mozilla-central/rev/e9fbcd30350b

This showed up on Linux debug and Linux 64 debug. The test failure in browser_privatebrowsing_protocolhandler.js showed up on Linux 64 opt.

s: talos-r3-fed-007
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 373235 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 140 instances of AtomImpl with size 20 bytes each (2800 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 24 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of CSSImportRuleImpl with size 48 bytes each (96 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 46 instances of CSSImportantRule with size 16 bytes each (736 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 11 instances of CSSNameSpaceRuleImpl with size 44 bytes each (484 bytes total)
TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_protocolhandler.js | application timed out after 330 seconds with no output
PROCESS-CRASH | chrome://mochikit/content/browser/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_protocolhandler.js | application crashed (minidump found)
Thread 0 (crashed)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | missing output line for total leaks!
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This was not the cause of the orange, and should be relanded.
Status: REOPENED → ASSIGNED
Keywords: checkin-needed
Target Milestone: mozilla2.0b7 → mozilla2.0b6
http://hg.mozilla.org/mozilla-central/rev/8cbd50bdeaee
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Backed out again because of leaks...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 477645 [details] [diff] [review]
Patch (v2)

The leaks were coming from the test, as I suspected.  This patch fixes this.
Attachment #474760 - Attachment is obsolete: true
Attachment #474884 - Attachment is obsolete: true
Attachment #477645 - Flags: review?(roc)
Attachment #477645 - Flags: approval2.0?
Comment on attachment 477645 [details] [diff] [review]
Patch (v2)

Isn't the fact that that test caused leaks a bug in our printpreview code or thereabouts? Better file it.
Attachment #477645 - Flags: review?(roc)
Attachment #477645 - Flags: review+
Attachment #477645 - Flags: approval2.0?
Attachment #477645 - Flags: approval2.0+
(In reply to comment #17)
> Isn't the fact that that test caused leaks a bug in our printpreview code or
> thereabouts? Better file it.

Filed bug 599080.
blocking2.0: ? → final+
Whiteboard: [needs landing]
Status: REOPENED → ASSIGNED
So I was mistaken in thinking that the version 2 of this patch doesn't leak.  It indeed does.  It seems like the contenteditable attribute on the input element, and having the |page-break-before:left;| set is what triggers the leak...
Whiteboard: [needs landing]
(Reporter)

Comment 20

7 years ago
Didn't this always leak. Afaik, it's normal that print preview leaks.
(In reply to comment #20)
> Didn't this always leak.

Yes, the leak is not a result of my patch, it was only uncovered by the test I added.

> Afaik, it's normal that print preview leaks.

What do you mean it's normal?  We shouldn't be leaking, should we?
(Reporter)

Comment 22

7 years ago
Sorry, I didn't mean it's normal, just that there are probably many more cases of leaking in print preview.
(In reply to comment #22)
> Sorry, I didn't mean it's normal, just that there are probably many more cases
> of leaking in print preview.

That might be the case, but this particular leak has to be addressed before I can land this patch anyway...
(Reporter)

Comment 24

7 years ago
Well, as long as it gets fixed before 2.0, then, I guess.
It would be bad if this would not get fixed, because the patch gets the code back into a state where it's leaking instead of crashing.
OK, I finally figured this leak out!  It's related to leaking frames if we fail to inject them in the frame tree.  I'm going to post a patch in bug 599080, so this bug will be dependent on bug 599080.
Depends on: 599080
Whiteboard: [has patch][waiting on bug 599080]
Whiteboard: [has patch][waiting on bug 599080] → [needs landing]
http://hg.mozilla.org/mozilla-central/rev/d0fd6feceb80
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: mozilla2.0b6 → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Crash Signature: [@ nsTextEditorState::InitializeKeyboardEventListeners]
You need to log in before you can comment on or make changes to this bug.