340966 - ASSERT: Node QI Failed When middle clicking a bookmark.

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ryan Jones-Ward [:sciguyryan]]

When middle clicking a bookmark, a "ASSERT: Node QI Failed" box pops up with the following.

ASSERT: Node QI Failed
Stack trace:
0:QI_node([xpconnect wrapped nsINavHistoryResultNode],nsINavHistoryFolderResultNode)
1:asFolder([xpconnect wrapped nsINavHistoryResultNode])
2:PC_openLinksInTabs()
3:BT_onClick([object MouseEvent])
4:onclick([object MouseEvent])

Only in latest trunk build, was not present before today.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060609 Minefield/3.0a1 - Build ID: 2006060904

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Error box

Comment 2

[Peter van der Woude [:Peter6]]

also a large # of warnings in the errorconsole

Comment 3

[Peter van der Woude [:Peter6]]

regressed from Bug 333734

Comment 4

[:Gavin Sharp [email: [e-mail]]]

*** Bug 340993 has been marked as a duplicate of this bug. ***

Comment 5

[(not reading, please use [e-mail] instead)]

accepting, I have a patch for the trunk that fixes this and also warns (under the proper circumstances) for the additional scenario that I forgot when I "fixed" bug #333734, which is selecting multiple items in the places UI and then doing open in tabs.

Comment 6

[(not reading, please use [e-mail] instead)]

Attachment: fix for regression and fix a scenario I forgot about.

Comment 7

[(not reading, please use [e-mail] instead)]

Attachment: same patch, but removed my dumps.

Comment 8

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 225096 [details] [diff] [review]
same patch, but removed my dumps.

r=[e-mail], 

although it'd be nice to rename "tabsToOpen" to something that included the word "count" or "number" since it's not immediately obvious from a quick scan that it isn't an array or something like that.

Comment 9

[(not reading, please use [e-mail] instead)]

Attachment: renamed parameter from tabsToOpen to numTabsToOpen and one other fix

Comment 10

[(not reading, please use [e-mail] instead)]

Attachment: renamed parameter from tabsToOpen to numTabsToOpen and one other fix

the correct patch

Comment 11

[(not reading, please use [e-mail] instead)]

ben, here's the issue additional change addresses.  the existing code ensured the container was open in order to get the count and to get all uri's in the container.  when I added the prompt though, the user would now notice that the container was open (if it wasn't open before.)

so I added code to restore the original state before we prompt, and then restore the state to open after we prompt.

the code to restore the original state at the very end is still there.

      // Open each uri in the folder in a tab.
      var index = firstIndex;
      asFolder(node);
      var wasOpen = node.containerOpen;
      node.containerOpen = true;
      var cc = node.childCount;

+     // restore the original state (temporarily) so that if we prompt
+     // the user, the will not see a change to the open state.
+     node.containerOpen = wasOpen;
      if (!this._confirmOpenTabs(cc))
         return;
+     // ensure the container is open, we'll restore it again
+     // to the original state when we are done
+     node.containerOpen = true;


      for (var i = 0; i < cc; ++i) {
        var childNode = node.getChild(i);
        if (this.nodeIsURI(childNode)) {
          // If there are tabs to load over, load the uri into the next tab.
          if (index < tabCount)
            tabPanels[index].loadURI(childNode.uri);
          // Otherwise, create a new tab to load the uri into.
          else
            browser.addTab(childNode.uri);
          ++index;
        }
      }
      node.containerOpen = wasOpen;

Comment 12

[Ria Klaassen (not reading all bugmail)]

*** Bug 341065 has been marked as a duplicate of this bug. ***

Comment 13

[Phil Ringnalda (:philor)]

*** Bug 341213 has been marked as a duplicate of this bug. ***

Comment 14

[Michael Ventnor]

A message appears in the error console after the error:

Error: uncaught exception: [Exception... "Cannot modify properties of a
WrappedNative"  nsresult: "0x80570034 (NS_ERROR_XPC_CANT_MODIFY_PROP_ON_WN)" 
location: "JS frame :: chrome://browser/content/places/controller.js ::
PC_openLinksInTabs :: line 1338"  data: no]

I hope this helps.

Comment 15

[Hideo Oshima]

I can reproduce with trunk/Linux.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060611 Minefield/3.0a1

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 225134 [details] [diff] [review]
renamed parameter from tabsToOpen to numTabsToOpen and one other fix

>+      // restore the original state (temporarily) so that if we prompt
>+      // the user, the will not see a change to the open state.
>+      node.containerOpen = wasOpen;
>+      if (!this._confirmOpenTabs(cc))
>+        return;
>+      // ensure the container is open, we'll restore it again
>+      // to the original state when we are done
>+      node.containerOpen = true;

is this set to wasOpen later in the function? The patch doesn't show...

Comment 17

[(not reading, please use [e-mail] instead)]

> is this set to wasOpen later in the function? The patch doesn't show...

yes, see http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/controller.js#1418

Comment 18

[pal-moz]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060613 Minefield/3.0a1 ID:2006061318 [cairo]

WFM.
is this fixed ?

Comment 19

[(not reading, please use [e-mail] instead)]

> WFM. is this fixed ?

no, this is not fixed yet.

Comment 20

[pal-moz]

(In reply to comment #19)
> no, this is not fixed yet.
> 

ok,but for me, no alert dialog.
is this OS issue?
my PC : XP HOME SP2

Comment 21

[David Clark]

It only happens if I middle click a bookmark on the bookmark toolbar or menu.

Comment 22

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 225134 [details] [diff] [review]
renamed parameter from tabsToOpen to numTabsToOpen and one other fix

ok r=[e-mail]

Comment 23

[(not reading, please use [e-mail] instead)]

fix landed on the trunk.