loadOverlay fails to fire observer when overlay is overlaid

RESOLVED FIXED

Status

()

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: neil, Assigned: neil)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
If the overlay loaded document.loadOverlay itself has an overlay, either via a PI or via the chrome registry, then the overlay load observer never fires.

Note that this is not quite the same as bug 330458 as I am not trying to call document.loadOverlay before the previous overlay has loaded.
(Assignee)

Comment 1

11 years ago
Created attachment 305522 [details] [diff] [review]
Proposed patch

If you can think of a good way to notify for the original URI I'm all ears.

The only way I could think of was to prohibit multiple outstanding loads.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #305522 - Flags: superreview?(jst)
Attachment #305522 - Flags: review?(jst)

Updated

11 years ago
Attachment #305522 - Flags: superreview?(jst)
Attachment #305522 - Flags: superreview+
Attachment #305522 - Flags: review?(jst)
Attachment #305522 - Flags: review+
(Assignee)

Comment 2

11 years ago
Comment on attachment 305522 [details] [diff] [review]
Proposed patch

This means that, with care, loadOverlay can always be used usefully.
Attachment #305522 - Flags: approval1.9?
Comment on attachment 305522 [details] [diff] [review]
Proposed patch

a=beltzner for 1.9
Attachment #305522 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 4

11 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Backed this out as the possible cause of the Txul regression at least on Linux.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Didn't seem to be the cause of the Txul regression (appears to bug bug 395609), but still waiting to see if it's the cause of the Windows debug orange (due to an assertion).
Either this bug or bug 395609 (both backed out in the same cycle) was the cause of the orange on fxdebug-win32-tbox due to an assertion during the bloat test:

  WARNING: NS_ENSURE_TRUE(sgo || !hasHadScriptObject) failed: file e:/builds/tinderbox/Fx-Trunk-Memtest/WINNT_5.2_Depend/mozilla/content/base/src/nsContentUtils.cpp, line 2583
  WARNING: NS_ENSURE_TRUE(sgo || !hasHadScriptObject) failed: file e:/builds/tinderbox/Fx-Trunk-Memtest/WINNT_5.2_Depend/mozilla/content/base/src/nsContentUtils.cpp, line 2583
  ###!!! ASSERTION: invalid active window: 'Error', file e:/builds/tinderbox/Fx-Trunk-Memtest/WINNT_5.2_Depend/mozilla/embedding/components/windowwatcher/src/nsWindowWatcher.cpp, line 1086
  gklayout!nsFocusController::UpdateWWActiveWindow+0x000000000000013B (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\dom\src\base\nsfocuscontroller.cpp, line 592)
  gklayout!nsFocusController::SetActive+0x000000000000002E (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\dom\src\base\nsfocuscontroller.cpp, line 559)
  appshell!nsWebShellWindow::HandleEvent+0x0000000000000546 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\xpfe\appshell\src\nswebshellwindow.cpp, line 473)
  gkwidget!nsWindow::DispatchEvent+0x00000000000000C6 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\widget\src\windows\nswindow.cpp, line 972)
  gkwidget!nsWindow::DispatchWindowEvent+0x0000000000000026 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\widget\src\windows\nswindow.cpp, line 993)
  gkwidget!nsWindow::DispatchFocus+0x00000000000000D2 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\widget\src\windows\nswindow.cpp, line 5976)
  gkwidget!nsWindow::ProcessMessage+0x000000000000101A (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\widget\src\windows\nswindow.cpp, line 4530)
  gkwidget!nsWindow::WindowProc+0x0000000000000178 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\widget\src\windows\nswindow.cpp, line 1187)
  USER32!LoadCursorW+0x0000000000004CF5
  USER32!LoadCursorW+0x0000000000004E86
  USER32!GetMessageW+0x000000000000009F
  USER32!GetClientRect+0x000000000000004A
  ntdll!KiUserCallbackDispatcher+0x000000000000002E
  gklayout!nsView::~nsView+0x00000000000001AA (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\view\src\nsview.cpp, line 274)
  gklayout!nsView::`scalar deleting destructor'+0x000000000000000F
  gklayout!nsIView::Destroy+0x000000000000002A (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\view\src\nsview.cpp, line 314)
  gklayout!nsFrame::Destroy+0x00000000000000D5 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\layout\generic\nsframe.cpp, line 510)
  gklayout!nsSplittableFrame::Destroy+0x000000000000002D (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\layout\generic\nssplittableframe.cpp, line 74)
  gklayout!nsContainerFrame::Destroy+0x000000000000017E (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\layout\generic\nscontainerframe.cpp, line 300)
  gklayout!ViewportFrame::Destroy+0x000000000000001E (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\layout\generic\nsviewportframe.cpp, line 69)
  gklayout!nsFrameManager::Destroy+0x0000000000000079 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\layout\base\nsframemanager.cpp, line 284)
  gklayout!PresShell::Destroy+0x0000000000000401 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\layout\base\nspresshell.cpp, line 1676)
  gklayout!DocumentViewerImpl::Destroy+0x000000000000059C (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\layout\base\nsdocumentviewer.cpp, line 1522)
  docshell!nsDocShell::Destroy+0x0000000000000303 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\docshell\base\nsdocshell.cpp, line 3654)
  appshell!nsXULWindow::Destroy+0x000000000000035A (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\xpfe\appshell\src\nsxulwindow.cpp, line 503)
  appshell!nsWebShellWindow::Destroy+0x000000000000014F (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\xpfe\appshell\src\nswebshellwindow.cpp, line 840)
  appshell!nsContentTreeOwner::Destroy+0x0000000000000047 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\xpfe\appshell\src\nscontenttreeowner.cpp, line 563)
  gklayout!nsGlobalWindow::ReallyCloseWindow+0x000000000000021B (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\dom\src\base\nsglobalwindow.cpp, line 5412)
  gklayout!nsCloseEvent::Run+0x0000000000000024 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\dom\src\base\nsglobalwindow.cpp, line 5220)
  xpcom_core!nsThread::ProcessNextEvent+0x00000000000001FA (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\xpcom\threads\nsthread.cpp, line 511)
  xpcom_cnsStringStats
   => mAllocCount:          33007
   => mReallocCount:         4184
   => mFreeCount:           27642  --  LEAKED 5365 !!!
   => mShareCount:          29820
   => mAdoptCount:           4329
   => mAdoptFreeCount:       4294  --  LEAKED 35 !!!
  ore!NS_ProcessNextEvent_P+0x0000000000000053 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\obj-fx-trunk\xpcom\build\nsthreadutils.cpp, line 227)
  gkwidget!nsBaseAppShell::Run+0x000000000000005D (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\widget\src\xpwidgets\nsbaseappshell.cpp, line 151)
  tkitcmps!nsAppStartup::Run+0x000000000000006B (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\toolkit\components\startup\src\nsappstartup.cpp, line 181)
  xul!XRE_main+0x0000000000003350 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\toolkit\xre\nsapprunner.cpp, line 3149)
  firefox!NS_internal_main+0x00000000000002B2 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\browser\app\nsbrowserapp.cpp, line 158)
  firefox!wmain+0x0000000000000119 (e:\builds\tinderbox\fx-trunk-memtest\winnt_5.2_depend\mozilla\toolkit\xre\nswindowswmain.cpp, line 87)
  firefox!__tmainCRTStartup+0x00000000000001A6 (f:\sp\vctools\crt_bld\self_x86\crt\src\crtexe.c, line 594)
  firefox!wmainCRTStartup+0x000000000000000D (f:\sp\vctools\crt_bld\self_x86\crt\src\crtexe.c, line 414)
  kernel32!ProcessIdToSessionId+0x0000000000000209


From the log, it also looks like more DOMWINDOWs were created than before in those broken cycles; something Jesse also saw in one of his builds, along with a bunch of other brokenness (but that went away when he updated).
(Assignee)

Comment 8

11 years ago
Hmm, well this code path shouldn't actually get triggered until we write some code that uses it ;-) Resolving fixed again for now.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
Reopening, this was backed out due to bug 420095.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #9)
> Reopening, this was backed out due to bug 420095.

What bug # did you really mean?

Updated

11 years ago
Blocks: 411215

Updated

11 years ago
Blocks: 398751
(Assignee)

Updated

11 years ago
Blocks: 419865
(Assignee)

Comment 12

11 years ago
Created attachment 307099 [details] [diff] [review]
Fixed patch

OK, so I hadn't noticed the while loop last time - this potentially invalidates mCurrentPrototype while looking for a loadable overlay. I also fixed the case where the overlay is already in the XUL cache (i.e. shouldReturn is false).
Attachment #305522 - Attachment is obsolete: true
Attachment #307099 - Flags: superreview?(djst)
Attachment #307099 - Flags: review?(djst)
(Assignee)

Comment 13

11 years ago
Created attachment 307100 [details] [diff] [review]
Fixed patch

OK, so I hadn't noticed the while loop last time - this potentially invalidates mCurrentPrototype while looking for a loadable overlay. I also fixed the case where the overlay is already in the XUL cache (i.e. shouldReturn is false).

This time, with the correct reviewer!
Attachment #307099 - Attachment is obsolete: true
Attachment #307100 - Flags: superreview?(jst)
Attachment #307100 - Flags: review?(jst)
Attachment #307099 - Flags: superreview?(djst)
Attachment #307099 - Flags: review?(djst)

Updated

11 years ago
Attachment #307100 - Flags: superreview?(jst)
Attachment #307100 - Flags: superreview+
Attachment #307100 - Flags: review?(jst)
Attachment #307100 - Flags: review+
(Assignee)

Updated

11 years ago
Attachment #307100 - Flags: approval1.9?
Comment on attachment 307100 [details] [diff] [review]
Fixed patch

a1.9=beltzner
Attachment #307100 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 15

11 years ago
Fixed fix checked in.
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Updated

10 years ago
Flags: in-testsuite?

Updated

10 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.