912433 - JavaScript Error: "this.controller is undefined" {file: "chrome://browser/content/places/browserPlacesViews.js" line: 216}]

Status: NEW

(Firefox :: Bookmarks & History, defect, P3)

Description

[David Teller [:Yoric] - still alive but not very active]

JavaScript strict warning: chrome://browser/content/places/browserPlacesViews.js, line 104: reference to undefined property this._controller
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "this.controller is undefined" {file: "chrome://browser/content/places/browserPlacesViews.js" line: 216}]' when calling method: [nsINavHistoryResultObserver::containerStateChanged]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/places/browserPlacesViews.js :: PlacesViewBase.prototype.result :: line 77"  data: yes]
************************************************************

I encounter this error reproducibly upon startup with latest m-c.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Set the controller before setting the place to avoid errors

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Looks like this has been around for some time now and was exposed by bug 895340, I think.

Comment 3

[Marco Bonardo [:mak]]

So, the patch is "scary" in the sense it may end up covering some unexpected behavior, for example doesn't make much sense to have a controller until there's no location. And _startMarker was supposed to always be defined.

First of all, since I recall this setup is quite crazy looping into itself, I tried to rebuild a pseudostack:

new PlacesMenu()
PlacesViewBase.call()
set place()
result.addobserver()
set result()
containerOpen = true
containerStateChanged
invalidateContainer

here the controller has not yet been created, it will be created after the place = location call in PlacesViewBase. So that's what looks like the first exception.

_rebuildPopup
_cleanPopup

here we try to use _startMarker that is defined by ensureMarkers() that is invoked by _onPopupShowing, that is called in the PlacesMenu constructor, but only after building PlacesViewBase.

First, I wonder how we entered the PlacesMenu constructor, comment 0 doesn't seem to point out the need to open a Places menu (history/bookmarks), is that a required step to reproduce?

I wonder if we should rather avoid opening the container in the result setter and instead open it at the end of the view constructor if the result is set... I need to think about it a little bit more.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Aww, I thought this would be an easy fix. Please, someone take over :)

(In reply to Marco Bonardo [:mak] from comment #3)
> First, I wonder how we entered the PlacesMenu constructor, comment 0 doesn't
> seem to point out the need to open a Places menu (history/bookmarks), is
> that a required step to reproduce?

Yes, the error occurs whenever I open the 'History' menu. My dev profile should be rather clean without any places related things. I can reproduce this 100% of the time.

Comment 6

[Mike de Boer [:mikedeboer]]

Now with the following stack:

        PVB__rebuildPopup chrome://browser/content/places/browserPlacesViews.js:261:1
	PVB_invalidateContainer chrome://browser/content/places/browserPlacesViews.js:725:7
	PVB_containerStateChanged chrome://browser/content/places/browserPlacesViews.js:662:7
	set result chrome://browser/content/places/browserPlacesViews.js:91:7
	set place chrome://browser/content/places/browserPlacesViews.js:63:5
	PlacesViewBase chrome://browser/content/places/browserPlacesViews.js:16:3
	PlacesMenu chrome://browser/content/places/browserPlacesViews.js:1751:3
	HistoryMenu chrome://browser/content/browser-places.js:714:3
	onpopupshowing chrome://browser/content/browser.xul:1:78

I'll try to investigate further/ take a look when I find a little time to do so.

Comment 7

[Mike de Boer [:mikedeboer]]

Also with the following stack:

        PVB__rebuildPopup chrome://browser/content/places/browserPlacesViews.js:261:1
	PVB_invalidateContainer chrome://browser/content/places/browserPlacesViews.js:725:7
	PVB_containerStateChanged chrome://browser/content/places/browserPlacesViews.js:662:7
	set result chrome://browser/content/places/browserPlacesViews.js:91:7
	set place chrome://browser/content/places/browserPlacesViews.js:63:5
	PlacesViewBase chrome://browser/content/places/browserPlacesViews.js:16:3
	PlacesMenu chrome://browser/content/places/browserPlacesViews.js:1751:3
	onpopupshowing chrome://browser/content/browser.xul:1:144
	waitForSyncCallback resource://services-common/async.js:98:7
	makeSpinningCallback/callback.wait resource://services-common/async.js:168:27
	promiseSpinningly resource://services-common/async.js:234:12
	_processIncoming resource://services-sync/engines.js:1289:18
	_processIncoming resource://services-sync/engines/history.js:62:14
	_sync resource://services-sync/engines.js:1798:7
	WrappedNotify resource://services-sync/util.js:161:21
	sync resource://services-sync/engines.js:723:5
	_syncEngine resource://services-sync/stages/enginesync.js:219:7
	sync resource://services-sync/stages/enginesync.js:166:15
	onNotify resource://services-sync/service.js:1080:7
	WrappedNotify resource://services-sync/util.js:161:21
	WrappedLock resource://services-sync/util.js:117:16
	_lockedSync resource://services-sync/service.js:1070:12
	sync/< resource://services-sync/service.js:1062:7
	WrappedCatch resource://services-sync/util.js:92:16
	sync resource://services-sync/service.js:1051:5
	sync self-hosted:951:17

(During a first-time sync with a large account.)