1525200 - Nightly: extension stopped inserting URLs in History

Status: RESOLVED FIXED

(WebExtensions :: General, defect)

Description

[Caspy7]

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.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

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:

• https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/browser/components/extensions/parent/ext-bookmarks.js#5

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

• https://searchfox.org/mozilla-central/rev/152993fa346c8fd9296e4cd6622234a664f53341/browser/components/extensions/parent/ext-history.js#5-6

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})()`,
   });

Comment 2

[:Gijs (he/him)]

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

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1525200 - Fix history webextension API failing to load when loaded after the bookmarks API. r?zombie!

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.

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1525200 - Add an xpcshell test which loads all the WebExtensions API modules. r?zombie!

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

[Pulsebot]

Pushed by [e-mail]:
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

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/87194c6797c2
https://hg.mozilla.org/mozilla-central/rev/9959d6380a99