Nightly: extension stopped inserting URLs in History

RESOLVED FIXED in Firefox 67

Status

defect
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: caspy77, Assigned: rpl)

Tracking

({regression})

unspecified
mozilla67
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox65 unaffected, firefox66 unaffected, firefox67+ fixed)

Details

Attachments

(2 attachments)

Reporter

Description

4 months ago

I shared this issue in #webextensions and was recommended to file a bug.

I'm on Nightly. I use an extension called Imagus ( https://addons.mozilla.org/en-US/firefox/addon/imagus/ ) that let's you hover the cursor over links to media such as images and see a larger preview of the target. One of the options [I have checked] inserts the URL of that item in history, thereby showing the link as visited.

Sometime in the last week or so this function stopped working. I double checked that the option was checked and tested in a fresh profile where it worked fine.
When I hover a link, this is what shows in the browser console: https://i.imgur.com/vYivqxF.png
I was told the bit about PlacesUtils looked "particularly interesting"

Feel free to ask me any followups.

Assignee

Comment 1

4 months ago

This looks like a regression related to Bug 1514594, which has recently applied a change to how ChromeUtils.import works and rewrote a bunch of usage of it over the mozilla-central tree.

The issue is being triggered if the bookmarks API module is being loaded before the history API module,
because both of them uses PlacesUtils but ext-bookmarks.js imports it using ChromeUtils import:

whereas ext-history.js defines it as a lazy module getter:

When ext-bookmarks API is being loaded by an extension which is starting up before the extension that uses the history API (e.g. the extension linked in comment 0), PlacesUtils is initially defined as a non-configurable property by ext-bookmarks API, and then ext-history.js calls ChromeUtils.defineLazyGetter which tries to redefine it and fails by raising the following exception:

Extension error: can't redefine non-configurable property "PlacesUtils" chrome://browser/content/parent/ext-history.js:5 :: @chrome://browser/content/parent/ext-history.js:5:1

To reproduce this issue and double-check that it is actually behaving as described above, the following changes can be temporarily applied to the existing automated test for the ext-history.js API module:

diff --git a/browser/components/extensions/test/xpcshell/test_ext_history.js b/browser/components/extensions/test/xpcshell/test_ext_history.js
--- a/browser/components/extensions/test/xpcshell/test_ext_history.js
+++ b/browser/components/extensions/test/xpcshell/test_ext_history.js
@@ -18,6 +18,8 @@ add_task(async function test_delete() {
     let historyClearedCount = 0;
     let removedUrls = [];
 
+    browser.bookmarks.getTree();
+
     browser.history.onVisitRemoved.addListener(data => {
       if (data.allHistory) {
         historyClearedCount++;
@@ -54,7 +56,7 @@ add_task(async function test_delete() {
 
   let extension = ExtensionTestUtils.loadExtension({
     manifest: {
-      permissions: ["history"],
+      permissions: ["bookmarks", "history"],
     },
     background: `(${background})()`,
   });
Blocks: 1514594

Comment 2

4 months ago

[Tracking Requested - why for this release]:
Broken webextension compat which should be fixed before release.

Assignee

Comment 3

4 months ago

This patch change ext-bookmarks.js to define PlaceUtils as a lazy module getter,
as there are other two API modules (ext-history.js and ext-browsingData.js) are also using the same
JSM module and they are defining it as a lazy module getter, which would raise an error and breaks
those two WebExtensions APIs if the ext-bookmarks.js gets loaded first.

Assignee

Comment 4

4 months ago

This patch defines a new xpcshell test which loads all the WebExtensions API modules twice,
first in alphabetic order and then again in reversed order.

The goal of this new test is to make it easier to detect issues with API module loading,
because all these API modules are loaded lazily in a shared global and it can fail
if multiple API modules defines the same global in incompatible ways (e.g. something which
is defined as a const in one module, and then as a var in a different one).

Depends on D18683

Comment 5

4 months ago
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/87194c6797c2
Fix history webextension API failing to load when loaded after the bookmarks API. r=zombie
https://hg.mozilla.org/integration/autoland/rev/9959d6380a99
Add an xpcshell test which loads all the WebExtensions API modules. r=zombie

Comment 6

4 months ago
bugherder
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Assignee: nobody → lgreco
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.