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

Status: VERIFIED FIXED

(Core :: XBL, defect, P1)

Description

[Martijn Wargers (dead)]

Attachment: iframe src needed for testcase

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..

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Maybe bug 396108 is related?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w.

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).

Comment 5

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Need to get the test in too...

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, and the failure was Mac-only.

Comment 9

[Robert Strong (they/them - no direct email)]

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?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Filed bug 396839 on the Linux debug failures.

Thanks for looking into this!

Comment 11

[Dave Townsend [:mossop]]

Attachment: 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

Comment 12

[Dave Townsend [:mossop]]

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.

Comment 13

[Boris Zbarsky [:bzbarsky]]

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?

Comment 14

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007092105 Minefield/3.0a8pre

Comment 15

[[e-mail]]

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.