[:philipp] Reporter
	Description • 8 years ago

This bug was filed from the Socorro interface and is 
report bp-688c92be-5c16-4a87-afe8-ac0512160816.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsBaseWidget::AddChild(nsIWidget*) 	widget/nsBaseWidget.cpp:608
1 	xul.dll 	nsWindow::SetParent(nsIWidget*) 	widget/windows/nsWindow.cpp:1036
2 	xul.dll 	nsPluginFrame::PrepForDrawing(nsIWidget*) 	layout/generic/nsPluginFrame.cpp:308
3 	xul.dll 	nsPluginInstanceOwner::SetFrame(nsPluginFrame*) 	dom/plugins/base/nsPluginInstanceOwner.cpp:3795
4 	xul.dll 	nsObjectLoadingContent::HasNewFrame(nsIObjectFrame*) 	dom/base/nsObjectLoadingContent.cpp:1314
5 	xul.dll 	nsPluginFrame::DidReflow(nsPresContext*, nsHTMLReflowState const*, nsDidReflowStatus) 	layout/generic/nsPluginFrame.cpp:853
6 	xul.dll 	nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) 	layout/generic/nsLineLayout.cpp:1066
7 	xul.dll 	nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) 	layout/generic/nsBlockFrame.cpp:4088
8 	xul.dll 	nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) 	layout/generic/nsBlockFrame.cpp:3890
9 	xul.dll 	nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) 	layout/generic/nsBlockFrame.cpp:3756
10 	xul.dll 	nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) 	layout/generic/nsBlockFrame.cpp:2754
11 	xul.dll 	GetNormalLineHeight 	layout/generic/nsHTMLReflowState.cpp:2716
12 	xul.dll 	nsHTMLReflowState::CalcLineHeight() 	layout/generic/nsHTMLReflowState.cpp:2779
13 	xul.dll 	nsBlockFrame::ReflowPushedFloats(nsBlockReflowState&, nsOverflowAreas&, unsigned int&) 	layout/generic/nsBlockFrame.cpp:6317
14 	xul.dll 	nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) 	layout/generic/nsBlockFrame.cpp:1147
15 		@0x179feb 	
16 		@0x17998f 	
17 	xul.dll 	nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) 	layout/generic/nsBlockFrame.cpp:2751
18 	xul.dll 	nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) 	layout/generic/nsBlockFrame.cpp:2290
19 	xul.dll 	nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) 	layout/generic/nsBlockFrame.cpp:1171

this crash signature seems to be regressing since firefox 48 builds and is happening with MOZ_RELEASE_ASSERT(!aChild->GetNextSibling() && !aChild->GetPrevSibling()) (aChild not properly removed from its old child list) that was added in bug 1174461.

currently this is making up 0.11% of all browser crashes on 48.0

	Sylvestre Ledru [:Sylvestre]
	Comment 1 • 8 years ago

Low volume, wontfix for 48 but happy to take a patch in 49

	Benjamin Smedberg
	Comment 2 • 8 years ago

Jim, bug 1174461 was yours: is it likely that this is a problem for windowed-mode plugins only? If so, I'm not sure I care much. But also, do we need a release assert here? What would happen if this code continued to run?

	Jim Mathies [:jimm] Assignee
	Comment 3 • 8 years ago

(In reply to Benjamin Smedberg [:bsmedberg] from comment #2)
> Jim, bug 1174461 was yours: is it likely that this is a problem for
> windowed-mode plugins only? If so, I'm not sure I care much. But also, do we
> need a release assert here? What would happen if this code continued to run?

Yes that was some work related to e10s windowed plugins. Looking at reports though we're hitting this outside of e10s. Lets see if we can remove the release asserts.

	Jim Mathies [:jimm] Assignee
	Comment 4 • 8 years ago

I experimented around a bit trying to reproduce this but didn't have any luck. The assert we're hitting is not associated with the original e10s related changes, it appears to be tied to reflow and happen in single process mode.

Note Flash 23 mitigates this issue for content with Flash.

	Benjamin Smedberg
	Comment 5 • 8 years ago

Comment on attachment 8788589 [details] [diff] [review]
revert asserts patch

Might want to use MOZ_ASSERT, but either way this is fine.

	Pulsebot
	Comment 6 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b982a2199355
Remove release asserts from nsBaseWidget::AddChild associated with existing sibling checks. r=bsmedberg

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 7 • 8 years ago

Backed out for build bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cf23f8fe42fb05e896d77f3a44eed8f61fb383e8

/home/worker/workspace/build/src/widget/nsBaseWidget.cpp:622:28: error: macro "NS_ASSERTION" requires 2 arguments, but only 1 given
/home/worker/workspace/build/src/widget/nsBaseWidget.cpp:623:47: error: macro "NS_ASSERTION" requires 2 arguments, but only 1 given
/home/worker/workspace/build/src/widget/nsBaseWidget.cpp:622:5: error: 'NS_ASSERTION' was not declared in this scope
gmake[5]: *** [nsBaseWidget.o] Error 1

	Pulsebot
	Comment 8 • 8 years ago

Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cc177529f5c
Backed out changeset b982a2199355 for build bustage. r=backout on a CLOSED TREE

	Jim Mathies [:jimm] Assignee
	Comment 9 • 8 years ago

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

	Pulsebot
	Comment 11 • 8 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5c1c15e0a74
Remove release asserts from nsBaseWidget::AddChild associated with existing sibling checks. r=bsmedberg

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 8 years ago
bugherder

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

	Marco Castelluccio [:marco]
	Comment 13 • 8 years ago

Should we uplift to 51? The fix looks pretty safe.

	Jim Mathies [:jimm] Assignee
	Comment 14 • 8 years ago

Comment on attachment 8798425 [details] [diff] [review]
revert asserts patch

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1174461 
[User impact if declined]:
crashy browser due to diagnostic release asserts
[Is this code covered by automated tests?]:
yes
[Has the fix been verified in Nightly?]:
yes
[Needs manual test from QE? If yes, steps to reproduce]: 
no
[List of other uplifts needed for the feature/fix]:
none
[Is the change risky?]:
no
[Why is the change risky/not risky?]:
removing release asserts and replacing them with debug asserts
[String changes made/needed]:
none

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 15 • 8 years ago

Comment on attachment 8798425 [details] [diff] [review]
revert asserts patch

Crash fix, it's been in 52 for a while, let's uplift to beta.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/b1fde3bfa659