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

Status: RESOLVED FIXED

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

Description

[Alice0775 White]

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

Comment 1

[Alice0775 White]

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

[Marco Bonardo [:mak]]

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

[Carsten Book [:Tomcat]]

duping to Bug 527673 because of the same assertion

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Its more a dupe of bug 525480

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Lets reopen this bug. It has better steps and I can reproduce it.

Comment 7

[Marco Bonardo [:mak]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 9

[Asaf Romano (gone)]

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.

Comment 10

[Asaf Romano (gone)]

Attachment: lovely

Comment 11

[Asaf Romano (gone)]

As for why it did "work" before: We just bailed out and didn't assert. The notifications were apparently always there.

Comment 12

[Asaf Romano (gone)]

This breaks the chevron as well.  Just open the chevron popup before removing the bookmarks toolbar.

Comment 13

[Asaf Romano (gone)]

My previous comment might explain the assertions we saw with PMV_* (The bookmarks/history menus are never removed AFAICT).

Comment 14

[Asaf Romano (gone)]

Oh, and we also update the chevron-view multiple time after toolbar customization.

Comment 15

[Asaf Romano (gone)]

Attachment: the chevron is lovely too

Comment 16

[Marco Bonardo [:mak]]

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

[Marco Bonardo [:mak]]

This should block, we had reports from different users, once something breaks we will keep asserting for any further action (visits, bookmarks).

Comment 18

[Marco Bonardo [:mak]]

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

[Dietrich Ayala (:dietrich)]

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.

Comment 20

[Marco Bonardo [:mak]]

Comment on attachment 412555 [details] [diff] [review]
the chevron is lovely too

waiting new patch.

I guess would be hard to test this automatically.

Comment 22

[Marco Bonardo [:mak]]

this should _at_least_ go into 3.6.1

Comment 24

[Marcia Knous [:marcia]]

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

[Marco Bonardo [:mak]]

Mano, could you provide an updated patch please? this should have already been fixed on central, so we can get some testing on it.

Comment 26

[Asaf Romano (gone)]

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

Comment 27

[Asaf Romano (gone)]

Attachment: patch

Comment 28

[Marco Bonardo [:mak]]

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

[Marco Bonardo [:mak]]

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

[Marco Bonardo [:mak]]

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

[Marco Bonardo [:mak]]

Comment on attachment 418631 [details] [diff] [review]
patch

r- due to problem reported, see comment 30.

Comment 33

[u88484]

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

[Gaydov Victor]

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

[Will Roberts [:bws42]]

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

[Will Roberts [:bws42]]

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

[Dietrich Ayala (:dietrich)]

recent reports, maybe more fallout from bug 556739?

Comment 40

[[not reading bugmail]]

(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

[Marco Bonardo [:mak]]

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

[Marco Bonardo [:mak]]

I think this has been fixed with bug 528884, Mano?

Comment 43

[Asaf Romano (gone)]

Yeah, and I don't think that it should be fixed it for 1.9.2.  If you think otherwise, please reopen.

Comment 44

[Marco Bonardo [:mak]]

No I think we really don't want THAT bug on 1.9.2. Thanks.