wincohi / bookmark-notes

a firefox webextension to bring back pre-62 bookmark annotations
Mozilla Public License 2.0
23 stars 4 forks source link

Pre-Import Parsing Dies with bookmarkElement null #16

Closed jscher2000 closed 6 years ago

jscher2000 commented 6 years ago

Getting:

TypeError: bookmarkElement is null options.js:141:1
importInit/< moz-extension://.../pages/options.js:141:1
forEach self-hosted:268:13 
importInit moz-extension://.../pages/options.js:138:7

For some reason my export files shows this when I probe it in the Web Console:

document.querySelectorAll('dt').length
6164
document.querySelectorAll('dt a').length
5464

I don't know what those 700 DTs are that aren't bookmarks. Maybe folders?? Anyway, I think options.js need to make sure bookmarkElement is not null before trying to get its href.

wincohi commented 6 years ago

Thank you for the report!

Version 1.1.3 was just pushed to AMO and should fix the issue. In particular, bceb8124915020a12c7747c64b0bbb1e37164903 should address this. Can you confirm whether this is fixed on your end?

jscher2000 commented 6 years ago

That issue definitely is resolved.

I do get errors importing my file for obscure reasons which perhaps should not affect most users. The function dies at:

Error: Type error for parameter query (Value must either: be a string value, or .url must match the format "url") for bookmarks.search. options.js line 119

Here were the culprits:

Local Files

(1) "file:///C:/Users/username/Favorites/"

I'm not clear on the reason this triggers an error.

(2) "jar:file:///C:/Program%20Files%20(x86)/Mozilla%20Firefox/browser/omni.ja!/"

This folder is obsolete in 64-bit Firefox, so it's a "file not found" and perhaps that is the issue?

chrome: protocol pages

This doesn't seem to affect all chrome: URIs, but some of them.

(3) "chrome://browser/content/preferences/cookies.xul"

This dialog was removed recently, so perhaps that causes the issue? Same issue for "chrome://browser/content/search/engineManager.xul". Same issue for bookmark to page of removed extension "chrome://norwell/content/norwell.xul".

(4) "chrome://pippki/content/exceptionDialog.xul"

This XUL page, and "chrome://browser/content/browser.xul" which triggers the same, still exist, so there is something else going on there.

I don't know whether you can detect something strange happening with searchParams before calling browser.bookmarks.search() or whether there is a better way to handle these cases.

wincohi commented 6 years ago

In an earlier commit, I added a filter to prevent the parser from trying to call browser.bookmarks.search() on any "URL" that began with about: or javascript:.

When using the aforementioned API, it will return an error if the url parameter doesn't match an internet protocol scheme or links to something the extension can't access for security reasons—though the documentation doesn't say anything specific about this, so I found out the hard way. It sounds like I'll just have to add (jar:)file:/// and chrome:// to the filter.

In any case, I'll go ahead and mark this issue as fixed; any more issues with the url parameter should probably be in a separate one.

jscher2000 commented 6 years ago

It sounds like I'll just have to add (jar:)file:/// and chrome:// to the filter.

That is simplest. Some people might have notes on a file: URI, but perhaps not very many.