Closed Bug 396587 Opened 15 years ago Closed 15 years ago

[FIX]Crash [@ PresShell::ResizeReflow] with binding, changing iframe width and removing iframe

Categories

(Core :: XBL, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9beta1

People

(Reporter: martijn.martijn, Assigned: bzbarsky)

References

Details

(4 keywords)

Crash Data

Attachments

(5 files)

See upcoming testcase, which crashes current trunk build directly, or after a few reloads.

Regarding the "iframe src needed for testcase", the second binding consists of this:
<bindings xmlns="http://www.mozilla.org/xbl">
<binding id="a">
<implementation>
<constructor>
  window.frameElement.parentNode.removeChild(window.frameElement);
</constructor>
</implementation>
<content>
<children xmlns="http://www.mozilla.org/xbl"/>
</content>
</binding>
</bindings>
The rest are basically just empty bindings.

This regressed between 2007-09-14 and 2007-09-15:
I guess a regression from bug 394014, somehow.

http://crash-stats.mozilla.com/report/index/dde6e91c-6619-11dc-b171-001a4bd43e5c
0  	PresShell::ResizeReflow(int, int)  	 mozilla/layout/base/nsPresShell.cpp:2503
1 	PresShell::ResizeReflow(nsIView*, int, int) 	mozilla/layout/base/nsPresShell.cpp:5806
2 	nsViewManager::DoSetWindowDimensions(int, int) 	mozilla/view/src/nsViewManager.h:279
3 	nsViewManager::SetWindowDimensions(int, int) 	mozilla/view/src/nsViewManager.cpp:365
4 	nsViewManager::DispatchEvent(nsGUIEvent*, nsEventStatus*) 	mozilla/view/src/nsViewManager.cpp:960
5 	HandleEvent 	mozilla/view/src/nsView.cpp:168
6 	nsWindow::DispatchEvent(nsGUIEvent*, nsEventStatus&) 	mozilla/widget/src/windows/nsWindow.cpp:1075
7 	nsWindow::DispatchWindowEvent(nsGUIEvent*) 	mozilla/widget/src/windows/nsWindow.cpp:1095
8 	nsWindow::OnResize(nsRect&) 	mozilla/widget/src/windows/nsWindow.cpp:5798
9 	nsWindow::ProcessMessage(unsigned int, unsigned int, long, long*) 	mozilla/widget/src/windows/nsWindow.cpp:4803
10 	nsWindow::WindowProc(HWND__*, unsigned int, unsigned int, long) 	mozilla/widget/src/windows/nsWindow.cpp:1288
11 	InternalCallWinProc 	
etc..
Attached file testcase
Maybe bug 396108 is related?
Attached patch Proposed fixSplinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attached patch Same as diff -w.Splinter Review
I think this same crash could have been triggered before, with a bit more work.  Fix has a few parts:

1)  Resize the subdoc in a post-reflow callback so we don't run script during
    reflow.
2)  Hold strong ref to document viewer across said resize.
3)  Fix some presshell code to deal well with DidDoReflow() running script
    (as it may).
Attachment #281425 - Flags: superreview?(roc)
Attachment #281425 - Flags: review?(roc)
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: Crash [@ PresShell::ResizeReflow] with binding, changing iframe width and removing iframe → [FIX]Crash [@ PresShell::ResizeReflow] with binding, changing iframe width and removing iframe
Target Milestone: --- → mozilla1.9 M9
Attachment #281425 - Flags: superreview?(roc)
Attachment #281425 - Flags: superreview+
Attachment #281425 - Flags: review?(roc)
Attachment #281425 - Flags: review+
Attachment #281425 - Flags: approval1.9+
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Need to get the test in too...
Flags: in-testsuite?
I backed this out because it seems to have caused orange in the regression test for bug 299716:

*** exiting
*** CHECK FAILED: 0.1 == 0.2
JS frame :: /Users/robcee/slave/trunk_osx2/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_throw :: line 99
JS frame :: /Users/robcee/slave/trunk_osx2/mozilla/tools/test-harness/xpcshell-simple/head.js :: do_check_eq :: line 114
JS frame :: ../../../../_tests/xpcshell-simple/test_extensionmanager/unit/test_bug299716.js :: do_check_item :: line 307
JS frame :: ../../../../_tests/xpcshell-simple/test_extensionmanager/unit/test_bug299716.js :: run_test_pt4 :: line 445
JS frame :: ../../../../_tests/xpcshell-simple/test_extensionmanager/unit/test_bug299716.js :: onStateChange :: line 188
native frame :: <unknown filename> :: <TOP_LEVEL> :: line 0
JS frame :: file:///Users/robcee/slave/trunk_osx2/mozilla/objdir/dist/bin/components/nsExtensionManager.js :: anonymous :: line 5303
JS frame :: file:///Users/robcee/slave/trunk_osx2/mozilla/objdir/dist/bin/components/nsExtensionManager.js :: anonymous :: line 5491
*** FAIL *** 

I can't make head or tail of that test, or why this patch would affect it, and furthermore the extension manager tests fail spectacularly in any debug build (assert city), so I don't even get this in my own builds.

I doubt I'll be able to reland this without some help with this test.  :(
Status: RESOLVED → REOPENED
Keywords: helpwanted
Resolution: FIXED → ---
Flags: blocking1.9?
Oh, and the failure was Mac-only.
Mac OS X and our startup is rather fragile - Bug 396234 for an example. EM tests don't fail in a debug Windows build and I'd appreciate it if you filed a bug for the failures you are seeing in debug builds.

Dave, since you have a Mac could you take a look at this?
Filed bug 396839 on the Linux debug failures.

Thanks for looking into this!
Attached file test failure log
I'm unable to reproduce the test failure locally. This is the log of the failure to save me wading through the tinderbox log in the future. Couple of suspicious looking items in there, no clue why this bug would have affected it though
Attachment #281647 - Attachment mime type: application/text → text/plain
The test failure is happening because the EM is failing to upgrade 2 extensions to newly downloaded versions. Specifically it appears to fail to move aside the old version of the extension. The failure code though is not anything I'd expect to ever see from how we call that function.
I relanded the patch, and now the tree is green.  No idea what happened the first time... maybe it was a network hiccup or something?  Or does this patch not use the network?
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007092105 Minefield/3.0a8pre
Status: RESOLVED → VERIFIED
Comment on attachment 281425 [details] [diff] [review]
Same as diff -w.

>+  nsSize innerSize(GetSize());
>+  if (IsInline()) {
>+    nsMargin usedBorderPadding = GetUsedBorderAndPadding();
>+    innerSize.width  -= usedBorderPadding.LeftRight();
>+    innerSize.height -= usedBorderPadding.TopBottom();
>+  }
This can be negative (<xul:iframe collapsed="true" style="padding: 1px;"> perhaps?) but the old code never tried to size its child widget to a negative size.
Depends on: 397871
Depends on: 399410
Crash Signature: [@ PresShell::ResizeReflow]
You need to log in before you can comment on or make changes to this bug.