Closed Bug 528006 Opened 12 years ago Closed 11 years ago

ASSERT: parent node must have _DOMElement set,When I carry out undoing of the bookmark


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






(Reporter: alice0775, Assigned: mano)



(Whiteboard: [fixed with bug 528884])


(1 file, 2 obsolete files)

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)
12:oncommand([object XULCommandEvent])

Expected Results:  
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
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)
Blocks: 498130
duping to Bug 527673 because of the same assertion
Closed: 12 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 527673
Its more a dupe of bug 525480
Duplicate of bug: 525480
Lets reopen this bug. It has better steps and I can reproduce it.
Resolution: DUPLICATE → ---
Ever confirmed: true
Duplicate of this bug: 525480
Henrik, are steps good enough to reproduce this reliably? can you provide a smaller set of steps? it's pretty important for us.
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

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.
Attached patch lovely (obsolete) — Splinter Review
Assignee: nobody → mano
Attachment #412519 - Flags: review?(mak77)
As for why it did "work" before: We just bailed out and didn't assert. The notifications were apparently always there.
This breaks the chevron as well.  Just open the chevron popup before removing the bookmarks toolbar.
My previous comment might explain the assertions we saw with PMV_* (The bookmarks/history menus are never removed AFAICT).
Oh, and we also update the chevron-view multiple time after toolbar customization.
Attached patch the chevron is lovely too (obsolete) — Splinter Review
Attachment #412519 - Attachment is obsolete: true
Attachment #412555 - Flags: review?(mak77)
Attachment #412519 - Flags: review?(mak77)
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())

>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?
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?
Depends on: 83635
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.
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 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)
Duplicate of this bug: 532336
this should _at_least_ go into 3.6.1
Duplicate of this bug: 528476
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.
Mano, could you provide an updated patch please? this should have already been fixed on central, so we can get some testing on it.
> 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.
Attached patch patchSplinter Review
Attachment #412555 - Attachment is obsolete: true
Attachment #418631 - Flags: review?(mak77)
Comment on attachment 418631 [details] [diff] [review]

>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.
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...
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]
Duplicate of this bug: 536876
Comment on attachment 418631 [details] [diff] [review]

r- due to problem reported, see comment 30.
Attachment #418631 - Flags: review?(mak77) → review-
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 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)
3:doTransaction([xpconnect wrapped nsITransaction])
4:placesTxn_doTransaction([xpconnect wrapped nsITransaction])
5:doTransaction([xpconnect wrapped nsITransaction])
6:PCH_bookmarkPage([object XULElement],undefined,false)
8:PSB_onClick([object MouseEvent])
9:onclick([object MouseEvent])
Duplicate of this bug: 540016
Blocks: 538080
Duplicate of this bug: 528433
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)
3:doTransaction([xpconnect wrapped nsITransaction])
4:placesTxn_doTransaction([xpconnect wrapped nsITransaction])
5:doTransaction([xpconnect wrapped nsITransaction])
6:anonymous([object XULCommandEvent])
7:oncommand([object XULCommandEvent])
10:onselect([object Event])
14:oncommand([object XULCommandEvent])
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.

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)
3:LLL_runBatched([xpconnect wrapped nsIFeedResult])
4:runInBatchMode([object Object],[xpconnect wrapped nsIFeedResult])
5:LLL_handleResult([xpconnect wrapped nsIFeedResult])
6:handleResult([xpconnect wrapped nsIFeedResult])
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)
recent reports, maybe more fallout from bug 556739?
(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.
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.
Blocks: 560198
I think this has been fixed with bug 528884, Mano?
Yeah, and I don't think that it should be fixed it for 1.9.2.  If you think otherwise, please reopen.
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed with bug 528884]
Depends on: 528884
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.