Closed
Bug 528006
Opened 15 years ago
Closed 15 years ago
ASSERT: parent node must have _DOMElement set,When I carry out undoing of the bookmark
Categories
(Firefox :: Bookmarks & History, defect, P1)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
People
(Reporter: alice0775, Assigned: asaf)
References
Details
(Whiteboard: [fixed with bug 528884])
Attachments
(1 file, 2 obsolete files)
10.29 KB,
patch
|
mak
:
review-
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b3pre) Gecko/20091111 Firefox/3.5.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.3a1pre) Gecko/20091111 Minefield/3.7a1pre ID:20091111045251
When I carry out undoing of the bookmark,
ASSERT dialog opens.
Reproducible: Always
Steps to Reproduce:
1. Start Minefield with new profile
2. View > Toolbars > Customize...
3. Drag and drop "Bookmarks Toolbar Items" from "Bookmarks Toolbar" to "Customize Toolbar" window
4. Done (Close "Customize Toolbar" window)
5. Delete Folder "Getting Started" from "Bookmarks Toolbar" Folder in Bookmarks menu
6. View > Toolbars > Customize... again
7. Drag and drop "Bookmarks" from "Customize Toolbar" window to "Bookmarks Toolbar"
8. Done (Close "Customize Toolbar" window)
9. Open Library (Ctrl+Shift+B)
10. Undo (Ctrl+Z)
Actual Results:
The following dialog opens.
ASSERT: parent node must have _DOMElement set
Stack Trace:
0:PMV_nodeInserted([object ResultNodeClassInfo],[object ResultNodeClassInfo],1)
1:insertBookmark(3,[xpconnect wrapped nsIURI],1,Getting Started)
2:PRIT_undoTransaction()
3:undoTransaction()
4:PAT_commit(true)
5:PAT_undoTransaction()
6:undoTransaction()
7:placesTxn_undoTransaction()
8:undoTransaction()
9:PC_doCommand(cmd_undo)
10:doCommand(cmd_undo)
11:goDoCommand(cmd_undo)
12:oncommand([object XULCommandEvent])
Expected Results:
No ASSERT
![]() |
Reporter | |
Comment 1•15 years ago
|
||
The same ASSERT shows in Namoroka.
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2b3pre) Gecko/20091111 Namoroka/3.6b3pre ID:20091111045315
Comment 2•15 years ago
|
||
if these steps are reproduceable they will help a lot, we are missing a way to reproduce these assertions, seen in other bugs (sometimes related to add-ons)
Comment 3•15 years ago
|
||
duping to Bug 527673 because of the same assertion
Status: UNCONFIRMED → RESOLVED
Closed: 15 years ago
Resolution: --- → DUPLICATE
Comment 4•15 years ago
|
||
Its more a dupe of bug 525480
Comment 5•15 years ago
|
||
Lets reopen this bug. It has better steps and I can reproduce it.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---
Updated•15 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 7•15 years ago
|
||
Henrik, are steps good enough to reproduce this reliably? can you provide a smaller set of steps? it's pretty important for us.
Comment 8•15 years ago
|
||
Just run the given steps and you will be able to reproduce it 100%. It will happen on all platforms. Here some lesser steps to run into the problem:
1. Open the bookmarks sidebar
2. Remove the bookmarks from the toolbar via Customize Toolbars
3. Remove the "Getting started" bookmark via the sidebar
4. Press Cmd/Ctrl+Z
5. Remove the bookmark again
That steps causes the assertion from the other bug we have. Looks like the undo component is broken.
OS: Windows Vista → All
Hardware: x86 → All
Assignee | ||
Comment 9•15 years ago
|
||
BLOODY HELL
I wasted the entire evening on moving the bookmarks toolbar to "addBinding"/"removeBinding" mode, just to find out that:
1. addBinding is quite broken
2. removeBinding is pretty much a no-op.
Assignee | ||
Comment 10•15 years ago
|
||
Assignee | ||
Comment 11•15 years ago
|
||
As for why it did "work" before: We just bailed out and didn't assert. The notifications were apparently always there.
Assignee | ||
Comment 12•15 years ago
|
||
This breaks the chevron as well. Just open the chevron popup before removing the bookmarks toolbar.
Assignee | ||
Comment 13•15 years ago
|
||
My previous comment might explain the assertions we saw with PMV_* (The bookmarks/history menus are never removed AFAICT).
Assignee | ||
Comment 14•15 years ago
|
||
Oh, and we also update the chevron-view multiple time after toolbar customization.
Assignee | ||
Comment 15•15 years ago
|
||
Attachment #412519 -
Attachment is obsolete: true
Attachment #412555 -
Flags: review?(mak77)
Attachment #412519 -
Flags: review?(mak77)
Comment 16•15 years ago
|
||
Comment on attachment 412555 [details] [diff] [review]
the chevron is lovely too
>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
> function BrowserCustomizeToolbar()
>
>+ // Bug 528006 (See comment in BrowserToolboxCustomizeDone).
I'm not sure makes sense to reference bug 528006, this is also one of the thousands issues that could be caused by bug 83635
the blame will be enough to go up to bug 528006, so i'd rather point to the real underlying bug and explain what we do here,
so "Cache this and that to deal with customization not destroying bindings correctly, bla bla. See comment in BrowserToolboxCustomizeDone for details."
>+ gCachedBTContent = document.getElementById("bookmarksBarContent");
>+ // Handle the chevron as well if it was already initialized.
>+ if (gCachedBTContent &&
>+ gCachedBTContent._chevronPopup.hasAttribute("type"))
>+ gCachedBTChevronPopup = gCachedBTContent._chevronPopup;
>+ else
>+ gCachedBTChevronPopup = null;
is there anything that prevents us from giving a real id to the chevron popup instead of using an internal property?
>@@ -3356,16 +3368,38 @@
> if (gURLBar)
> gURLBar.emptyText = gURLBarEmptyText.value;
>
> gProxyFavIcon = document.getElementById("page-proxy-favicon");
> gHomeButton.updateTooltip();
> gIdentityHandler._cacheElements();
> window.XULBrowserWindow.init();
>
>+ // Bug 528006:
ditto, looks useless to point to a single issue caused by a known underlying bug.
>+ // Yet another workaround for bug 83635: Destory the toolbar binding if
typo: destory
>+ // the bookmarks-toolbar has been removed.
here i'd put:
The bookmarks toolbar has been removed.
Notice that gCachedBTContent binding won't be accessible at all from now on.
>+ if (gCachedBTContent &&
>+ !document.getElementById("bookmarksBarContent") &&
>+ gCachedBTContent._result) {
>+ // No, gCachedBTContent binding is not accessible at all. However,
>+ // because "_result" isn't a xbl field, we can still access it and shut
>+ // the notifications.
and here "Since _resultNode and _result aren't XBL fields we can still access them to shut up the notifications."
>+ gCachedBTContent._resultNode.containerOpen = false;
>+ gCachedBTContent._result.viewer = null;
>+ delete gCachedBTContent._result;
>+ delete gCachedBTContent._resultNode;
>+
>+ if (gCachedBTChevronPopup) {
>+ gCachedBTChevronPopup._resultNode.containerOpen = false;
>+ gCachedBTChevronPopup._result.viewer = null;
>+ delete gCachedBTChevronPopup._result;
>+ delete gCachedBTChevronPopup._resultNode;
>+ }
>+ }
>+
all of this sounds so much as "internal code", it's a pity we can't provide an helper in the binding :(
Couldn't we add non-XBL helpers in the binding at contructor-time that are still accessible here? then this would become a more readable
if (gCachedBTContent && !gCachedBTContent.isAlive())
gCachedBTContent.lazyKill()
>diff --git a/browser/components/places/content/menu.xml b/browser/components/places/content/menu.xml
>- <field name="_initialized">false</field>
>+ <constructor><![CDATA[
>+ // See comment in the toolbar's ctor.
>+ if (this._result !== undefined) {
>+ // Reinitialize the controller.
>+ this._controller = new PlacesController(this);
>+ this.controllers.appendController(this._controller);
>+ }
>+ else {
>+ // Avoid "undefined" warnings. We cannot use xbl fields due
>+ // to to the bugs described in the toolbar's ctor.
this IS the contructor, where's the bug description?
What are "undefined" warnings?
Comment 17•15 years ago
|
||
This should block, we had reports from different users, once something breaks we will keep asserting for any further action (visits, bookmarks).
Flags: blocking-firefox3.6?
Comment 18•15 years ago
|
||
About blocking request: while assertions are a problem mostly for nightly testers, this bug has other unwanted effects for final release users:
a) a ghost binding is around and that can cause unwanted and unexpected effects on the real active view. We had a bunch of headaches due to these, so we highly prefer handling this case asap.
b) Performances will get worse and worse for each ghost binding based on number of bookmarks the user has.
c) if we have other bugs that could hit those assertions, this could cover them, and they could be pretty important to be fixed.
Comment 19•15 years ago
|
||
not a regression from 3.5 so i don't think this blocks. but does sound serious enough that we'd take a fix that is very well tested.
Flags: wanted-firefox3.6+
Flags: blocking-firefox3.6?
Flags: blocking-firefox3.6-
Priority: -- → P1
Comment 20•15 years ago
|
||
Comment on attachment 412555 [details] [diff] [review]
the chevron is lovely too
waiting new patch.
I guess would be hard to test this automatically.
Attachment #412555 -
Flags: review?(mak77)
Comment 22•15 years ago
|
||
this should _at_least_ go into 3.6.1
Comment 24•15 years ago
|
||
I get this error when testing Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2b5) Gecko/20091204 Firefox/3.6b5 GTB6 when I try to restore a JSON backup from 20091125. The assertion is pretty ugly since it fills the entire page.
Comment 25•15 years ago
|
||
Mano, could you provide an updated patch please? this should have already been fixed on central, so we can get some testing on it.
Assignee | ||
Comment 26•15 years ago
|
||
> is there anything that prevents us from giving a real id to the chevron popup
> instead of using an internal property?
Giving ids for anonymous nodes is a bad example, for starters.
Assignee | ||
Comment 27•15 years ago
|
||
Attachment #412555 -
Attachment is obsolete: true
Attachment #418631 -
Flags: review?(mak77)
Comment 28•15 years ago
|
||
Comment on attachment 418631 [details] [diff] [review]
patch
>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>+ // Yet another workaround for bug 83635: Destroy the toolbar binding if
uppercase after ":", should be a period i guess.
>+ // the bookmarks-toolbar has been removed. Notice that the bindings won't
bookmarks-toolbar should not need dash
>+ // be accessible at all from now on. killMeSoftly is defined manually
>+ // within the bindnigs' ctors.
typo: bindnigs
>+ if (gCachedBTContent &&
>+ !document.getElementById("bookmarksBarContent") &&
>+ gCachedBTContent._result) {
instead of checking _result, could we check ("killMeSoftly" in gCachedBTContent)?
looks like _result is already checked by the kill method and i'd prefer to not expose it being internal property
I'm still doing some testing since i've seen a strange behavior when removing the toolbar, closing customization panel, and then adding back the toolbar (something like customize getting disabled and never enabled again). i need to rebuild to check it's not my build's fault.
Comment 29•15 years ago
|
||
hm, i can reproduce this strange thing, once i drag bookmarks toolbat item into customize panel, and click "done", then if i right click on the toolbar "customize" option (and main menubar as well!) is disabled...
Comment 30•15 years ago
|
||
JavaScript error: , line 0: uncaught exception: [Exception... "Could not convert
JavaScript argument arg 1 [nsIDOMEventTarget.removeEventListener]" nsresult: "
0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS)" location: "JS frame :: chrome://brows
er/content/places/toolbar.xml :: anonymous :: line 96" data: no]
Comment 32•15 years ago
|
||
Comment on attachment 418631 [details] [diff] [review]
patch
r- due to problem reported, see comment 30.
Attachment #418631 -
Flags: review?(mak77) → review-
Comment 33•15 years ago
|
||
I see that Asaf seems to know what fix is needed but I just wanted to provide very simple steps to reproduce this.
Try to bookmark http://labs.adobe.com/technologies/flashplayer10/ via the location bar star, same assertion when removing the boomark via same dialog. The error or assertion looks a little different though.
ASSERT: parent node must have _DOMElement set
Stack Trace:
0:PMV_nodeInserted([object ResultNodeClassInfo],[object ResultNodeClassInfo],28)
1:insertBookmark(177,[xpconnect wrapped nsIURI],-1,Adobe Labs - Adobe Flash Player 10.1)
2:PCIT_doTransaction()
3:doTransaction([xpconnect wrapped nsITransaction])
4:placesTxn_doTransaction([xpconnect wrapped nsITransaction])
5:doTransaction([xpconnect wrapped nsITransaction])
6:PCH_bookmarkPage([object XULElement],undefined,false)
7:PCH_bookmarkCurrentPage(false)
8:PSB_onClick([object MouseEvent])
9:onclick([object MouseEvent])
Comment 36•15 years ago
|
||
I get the same assertion after the following steps (using 'add bookmark here' extension):
1. click 'add bookmark here' somewhere in bookmarks menu
2. click new folder button and press enter
ASSERT: node must have _DOMElement set
Stack Trace:
0:PMV_nodeRemoved([object ResultNodeClassInfo],[object ResultNodeClassInfo],0)
1:moveItem(181,809,0)
2:PMIT_doTransaction()
3:doTransaction([xpconnect wrapped nsITransaction])
4:placesTxn_doTransaction([xpconnect wrapped nsITransaction])
5:doTransaction([xpconnect wrapped nsITransaction])
6:anonymous([object XULCommandEvent])
7:oncommand([object XULCommandEvent])
8:doCommand()
9:EIO_onFolderTreeSelect()
10:onselect([object Event])
11:selectEventsSuppressed(false)
12:selectItems()
13:EIO_newFolder()
14:oncommand([object XULCommandEvent])
Comment 37•15 years ago
|
||
Starting with yesterday's nightly, at least two of us are getting a number of similar exceptions simply by browsing around; not by bookmarking any pages.
See: http://img2.pict.com/35/1c/f1/3376112/0/untitled.png
Comment 38•15 years ago
|
||
Just noticed that my live bookmarks are currently hosed, and here's a text version of the trace:
ASSERT: node must have _DOMElement set
Stack Trace:
0:TV_V_nodeRemoved([object ResultNodeClassInfo],[object ResultNodeClassInfo],74)
1:removeFolderChildren(339731)
2:LS_deleteLivemarkChildren(339731)
3:LLL_runBatched([xpconnect wrapped nsIFeedResult])
4:runInBatchMode([object Object],[xpconnect wrapped nsIFeedResult])
5:LLL_handleResult([xpconnect wrapped nsIFeedResult])
6:handleResult([xpconnect wrapped nsIFeedResult])
7:FP_sendResult()
8:FP_endDocument()
9:onStopRequest([xpconnect wrapped nsIRequest],null,0)
10:FP_onStopRequest([xpconnect wrapped nsIRequest],null,0)
11:onStopRequest([xpconnect wrapped nsIRequest],null,0)
12:LLL_onStopRequest([xpconnect wrapped nsIRequest],null,0)
Comment 39•15 years ago
|
||
recent reports, maybe more fallout from bug 556739?
Comment 40•15 years ago
|
||
(In reply to comment #38)
> Just noticed that my live bookmarks are currently hosed
originally that looks like bug 527638 regressed? which btw landed the morning we started seeing this bug too.
(In reply to comment #11)
Also, bug 519479 looks interesting if a regression window counts for anything.
Comment 41•15 years ago
|
||
We still need Mano's patch here.
If you can reproduce the issue you should:
1. ensure this is not due to an extension, if this happens with Add bookmarks here 2, it's not our issue
2. provide steps that allow us to reproduce the problem, otherwise we would be investigating ghosts
It is possible recent failures were just due to a missing change in bug 556739 that will be fixed in next nightly, so in such a case it's sadly "expected" in yesterday's nightly. retest in next nightly please.
Comment 42•15 years ago
|
||
I think this has been fixed with bug 528884, Mano?
Assignee | ||
Comment 43•15 years ago
|
||
Yeah, and I don't think that it should be fixed it for 1.9.2. If you think otherwise, please reopen.
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Whiteboard: [fixed with bug 528884]
Comment 44•14 years ago
|
||
No I think we really don't want THAT bug on 1.9.2. Thanks.
You need to log in
before you can comment on or make changes to this bug.
Description
•