LiveClick (and LiveClick Lunar) Crashes v29 Topic is solved

[RealityRipple]

Do not use LiveClick or LiveClick Lunar with Pale Moon 29. The crash I ran into on trunk (end of post #24 through post #28) was caused by changes which made it to this release. No estimate on a fix at this time.

[billmcct]

Yeah, found that out early this morning. I also tried the old version too, which I had never uninstalled, only disabled.
I was going to get around to notifying you of this but forgot with all the other problems I have been trying to sort out with v29.

[moonbat]

Please fix this
It's one of my favorite extensions and I use it to stay notified of forum replies on other forums that have RSS support.

[Moonchild]

does it actually crash the browser (appcrash/segfault)? If so then we need to drop this on the blocklist.

[RealityRipple]

I've already released a new version of LiveClick Lunar with strictCompatibility enabled to prevent it running on 29.

[New Tobin Paradigm]

People will just edit the install manifest. Why don't you fix it instead?

[RealityRipple]

It was a quick stop-gap to prevent people from making a bunch of "my browser crashed" posts and bug reports.

I don't think I can truly "fix" it. For now, I've prevented the crash by commenting out a line and have published that for v29 compatibility, but I'm not actually certain what that line accomplishes in the extension or why it's crashing the browser, or if other calls to the same function might trigger additional crashes that I just haven't exposed yet. I gave up my research on the crash when I realized it wasn't part of the patch I was working on at the time.


If anyone notices a functionality change or break or any other odd behavior in the new version, PLEASE LET ME KNOW! I'm a bit busy to delve into it at the moment.

[New Tobin Paradigm]

Crash dump might be worthwhile or failing that sure fire steps to reproduce and I can generate a dump and give it to Moonchild which can actually debug it.. If it crashes on the Windows.

[sciuro]

Thank you for the "fix". Just installed your LiveClick Lunar as I can not imagine a browser without LiveClick.
For now I can click on Bookmarks again without any crash and the LiveBookmarks seems working good.
To reproduce the crash: install LiveClick (caa:addon/liveclick) on a fresh PaleMoon v29.0.0 in Windows x64 (2004-OS Build 19041.746) and click on Bookmarks.

[RealityRipple]

Opening any bookmark folders in the toolbars that contain live bookmarks also triggers the crash.

Basically anything that would trigger PlacesViewBase's _insertNewItemToPopup (from chrome://browser/content/places/browserPlacesViews.js) event gets a dummy listener element attached to it after creation, and one of those two things results in a crash. If I recall from attempting to debug it, the object wasn't created yet or the internal pointer to it had been freed already when attaching the listener... or something along those lines.

---

I made a quick test script. Just this overlaid into browser.xul is enough to cause a crash:

Code: Select all

var CrashTestDummy =
{
 init: function()
 {
  let oldFun = PlacesViewBase.prototype._insertNewItemToPopup;
  PlacesViewBase.prototype._insertNewItemToPopup = function(aNewChild, aPopup, aBefore)
  {
   let element = oldFun.apply(this, arguments);
   if (!PlacesUtils.nodeIsFolder(aNewChild))
    return element;
   let test = document.createElement('box');
   element.appendChild(test);
   return element;
  }
 }
};
window.addEventListener('load', function() { CrashTestDummy.init(); }, false);

[Moonchild]

To reproduce the crash: install LiveClick (caa:addon/liveclick) on a fresh PaleMoon v29.0.0 in Windows x64 (2004-OS Build 19041.746) and click on Bookmarks.

I can't even install it; it says it's incompatible XD

[Moonchild]

I made a quick test script.

So you are effectively replacing the internal places function with something else. That's risky to begin with, regardless of what you're doing.
Could you wrap that test dummy up into an xpi overlaid in a way it crashes and PM it to me?

[RealityRipple]

I made a quick test script.

So you are effectively replacing the internal places function with something else. That's risky to begin with, regardless of what you're doing.
Could you wrap that test dummy up into an xpi overlaid in a way it crashes and PM it to me?

Replacing, but immediately calling the original before doing anything else. And I didn't write it; I'm just curating it.

[Moonchild]

I tried your crash test dummy xpi but it doesn't crash anything for me in my dev setup. :/
Yes I made sure to have a live bookmark in my bookmarks toolbar.

Replacing, but immediately calling the original before doing anything else.

And assuming it always succeeds by assigning it to a var and then adding children to the var without checking. Not the most safe way of going about it ;P

[RealityRipple]

Tested in a brand new profile on v29.0. Just trying to add a new live bookmark to the Bookmarks list instead of the toolbar crashed the browser. Same with trying to drag one from the toolbar into a folder on the toolbar. Used both Windows 10 and Arch Linux (gtk3).

---

Also just tried delaying and abstracting the call with:

Code: Select all

var CrashTestDummy =
{
 init: function()
 {
  let oldFun = PlacesViewBase.prototype._insertNewItemToPopup;
  PlacesViewBase.prototype._insertNewItemToPopup = function(aNewChild, aPopup, aBefore)
  {
   let element = oldFun.apply(this, arguments);
   if (!PlacesUtils.nodeIsFolder(aNewChild))
    return element;
   let lbl = element.getAttribute('label');
   let pops = element.parentElement;
   setTimeout(function(){
    for (let i = 0; i < pops.children.length; i++)
    {
     if (pops.children[i].tagName !== 'menu')
      continue;
     if (!pops.children[i].hasAttribute('label'))
      continue;
     if (pops.children[i].getAttribute('label') !== lbl)
      continue;
     let test = document.createElement('box');
     pops.children[i].appendChild(test);
     break;
    }
   }, 350);
   return element;
  }
 }
};
window.addEventListener('load', function() { CrashTestDummy.init(); }, false);

It just crashes after 350 ms instead. Not sure the override of _insertNewItemToPopup has anything to do with it. I'll try to narrow things down in the morning.

[Moonchild]

Ah there we go. finally. I added to the toolbar and it didn't crash.
And yes, it is indeed very much because you're telling the browser to append a child to something that is null. The node binding attempting to add something to null crashes (no surprise there).

EDIT: So it's pretty far into the CSS frame constructor code that this goes wrong, in nsCSSFrameConstructor::IsValidSibling which is passed a null aContent.
Avoiding this crash is easy, but I'm not sure what the effect would be on the calling code.

[Moonchild]

Crashfix:
https://repo.palemoon.org/MoonchildProd ... ff7b3599c3

[RealityRipple]

Thanks for that. I'll see if I can make the JS this extension uses a little... saner.

[billmcct]

I can not imagine a browser without LiveClick.

I personally have to have: JustOff's https://addons.palemoon.org/addon/respo ... s-toolbar/
so I don't have to click anything.

Edit:
@RealityRipple, Thanks for the update to LCL. It was the biggest problem I had with v29. After disabling it I had to fix two others also.

[billmcct]

@RealityRipple, One thing I have noticed is that the update to LCL no longer colors new posts.