All top-level functions in API files should be declared via const or let and should be uniquely named

RESOLVED FIXED in Firefox 55

Status

defect
P1
normal
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: svanderger, Assigned: bsilverberg)

Tracking

56 Branch
mozilla56
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox55 fixed, firefox56 fixed)

Details

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
Posted file bookmark-it.zip
User Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170618030207

Steps to reproduce:

1. Create new profile
2. Set up in about:config xpinstall.signatures.required;false
3. Install as permanent extensions "bookmark-it" and "HTTPS Everywhere webext"
   https://github.com/mdn/webextensions-examples/tree/master/bookmark-it
   https://www.eff.org/files/https-everywhere-test/index.html
4. Add to bookmarks this page
   https://developer.mozilla.org/en-US/Add-ons/WebExtensions
5. Restart browser
6. When HTTPS Everywhere enabled star is white; in browser console
   "Error: An unexpected error occurred  undefined"
7. Disable HTTPS Everywhere and restart browser
8. When HTTPS Everywhere disabled star is yellow
(Reporter)

Updated

2 years ago
Summary: Some strange incompatibilies between "HTTPS Everywhere - webext" and webextensions-example "bookmark-it" → Some strange incompatibilities between "HTTPS Everywhere - webext" and webextensions-example "bookmark-it"
Component: Untriaged → WebExtensions: Frontend
Product: Firefox → Toolkit
Summary: Some strange incompatibilities between "HTTPS Everywhere - webext" and webextensions-example "bookmark-it" → Some strange incompatibilies between "HTTPS Everywhere - webext" and webextensions-example "bookmark-it"
Summary: Some strange incompatibilies between "HTTPS Everywhere - webext" and webextensions-example "bookmark-it" → Some strange incompatibilities between "HTTPS Everywhere - webext" and webextensions-example "bookmark-it"
(Reporter)

Comment 1

2 years ago
(Reporter)

Comment 2

2 years ago

Updated

2 years ago
Flags: needinfo?(amckay)

Comment 3

2 years ago
I can reproduce this. HTTP Everywhere re-writes cookies to try to be HTTPS. The console shows an error in the ext-cookies.js code. 

However interestingly bookmarks doesn't use cookies at all. Not sure where the root cause of this lies yet though. Bob, would you be able to investigate a little more?
Flags: needinfo?(amckay) → needinfo?(bob.silverberg)
Priority: -- → P3
(Assignee)

Comment 4

2 years ago
I have figured out what the problem is, but not why it is happening. Here is what I have observed:

bookmarks.search is called from the bookmark-it extension, which runs the code at [1], which is:

`return PlacesUtils.bookmarks.search(query).then(result => result.map(convert));`

The search function in PlacesUtils.bookmarks runs and does return a result which is an array of bookmarks. In the `then` clause it should then convert those results using the `convert` function from ext-bookmarks.js at [2]. However, it is not using that `convert` function, it is using the `convert` function in ext-cookies.js at [3]. I have no idea why this is happening, but I assume it has something to do with the way APIs are loaded and complied because:

- it only happens when HTTPS Everywhere is enabled
- disabling HTTPS Everywhere does not fix the problem, except after a restart of the browser
- starting a browser without HTTPS Everywhere enabled does not cause the bug, but if HTTPS Everywhere is enabled during that browser session the bug starts to occur

This seems like a pretty major bug, but someone with more knowledge than me about how the APIs are loaded and complied needs to look into it further.

[1] http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/browser/components/extensions/ext-bookmarks.js#215-217
[2] http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/browser/components/extensions/ext-bookmarks.js#62
[3] http://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-cookies.js#13
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: P3 → P1
Summary: Some strange incompatibilities between "HTTPS Everywhere - webext" and webextensions-example "bookmark-it" → With "HTTPS Everywhere - webext" enabled, ext-bookmarks.js mistakenly uses the convert function from ext-cookies.js
(Assignee)

Comment 5

2 years ago
Kris pointed out that all of the API scripts get loaded into the same scope, so we need to make sure that the function names are unique, and also should declare them with `const` or `let` to generate errors when conflicts do occur.
Assignee: nobody → bob.silverberg
(Assignee)

Comment 6

2 years ago
Updating the bug title to reflect the problem and what needs to be done to fix it.
Flags: needinfo?(bob.silverberg)
Summary: With "HTTPS Everywhere - webext" enabled, ext-bookmarks.js mistakenly uses the convert function from ext-cookies.js → All top-level functions in API files should be declared via const or let and should be uniquely named
(Assignee)

Comment 7

2 years ago
I am working through all the files updating the function declarations, but I am a bit disturbed by the fact that this wasn't caught by any tests. Even after I update all the files, I assume that no conflicts will be caught by tests unless we load multiple APIs with conflicting function names into a single test.

Should we therefore have a test that loads *every* API to ensure that no conflicts exist?
Flags: needinfo?(kmaglione+bmo)
Loading every API wouldn't solve the problem. We'd need to load every API and then run all of the tests, but then we'd miss issues that only happen when not every API is loaded. I've considered eagerly loading all APIs in debug runs, which might be worth doing, but it's not my highest priority at the moment.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 9

2 years ago
Ok, so no test for now? This does seem serious enough to need uplift though, don't you think? Do you know which bug introduced this behaviour, where all API scripts are loaded into the same scope?
Flags: needinfo?(kmaglione+bmo)
(In reply to Bob Silverberg [:bsilverberg] from comment #9)
> This does seem serious enough to need uplift though, don't you think?

Yes.

> Do you know which bug introduced this behaviour, where all API scripts are
> loaded into the same scope?

Bug 1350522.
Flags: needinfo?(kmaglione+bmo)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

2 years ago
Just a note to myself for when this does get reviewed: I did a full try run of this at [1] and it all looks good.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=05145aefab22af701b1fd282b556c19715c6c28e

Comment 15

2 years ago
mozreview-review
Comment on attachment 8880119 [details]
Bug 1374237 - Part 1: Uniquify the name of the convert functions in ext-cookies.js and ext-bookmarks.js,

https://reviewboard.mozilla.org/r/151512/#review157808
Attachment #8880119 - Flags: review?(mixedpuppy) → review+

Comment 16

2 years ago
mozreview-review
Comment on attachment 8880120 [details]
Bug 1374237 - Part 2: Declare all top-level functions in toolkit API files files via const.,

https://reviewboard.mozilla.org/r/151514/#review157812
Attachment #8880120 - Flags: review?(mixedpuppy) → review+

Comment 17

2 years ago
mozreview-review
Comment on attachment 8880121 [details]
Bug 1374237 - Part 3: Declare all top-level functions in browser API files files via const.,

https://reviewboard.mozilla.org/r/151516/#review157826
Attachment #8880121 - Flags: review?(mixedpuppy) → review+
Lets also address the android files.
Flags: needinfo?(bob.silverberg)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

2 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> Lets also address the android files.

I've done this and pushed an additional commit to MozReview.
Flags: needinfo?(bob.silverberg)

Comment 24

2 years ago
mozreview-review
Comment on attachment 8881839 [details]
Bug 1374237 - Part 4: Declare all top-level functions in android API files files via const.,

https://reviewboard.mozilla.org/r/152914/#review158170
Attachment #8881839 - Flags: review?(mixedpuppy) → review+

Comment 25

2 years ago
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd6ecbda6ab2
Part 1: Uniquify the name of the convert functions in ext-cookies.js and ext-bookmarks.js, r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/20b6ab269217
Part 2: Declare all top-level functions in toolkit API files files via const., r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/6fe1b354f47f
Part 3: Declare all top-level functions in browser API files files via const., r=mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/019d09aaa261
Part 4: Declare all top-level functions in android API files files via const., r=mixedpuppy
(Assignee)

Comment 28

2 years ago
Approval Request Comment
[Feature/Bug causing the regression]: 1350522
[User impact if declined]: The bookmarks API is broken without this fix, and there may be other effects on WebExtensions APIs as well.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: This is just a refactor, renaming a few functions and declaring others via const. Everything touched is covered by tests and try runs are clean. A try against beta was done at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9511f5e3c2044a11a859ff22507fa273865884c.
[String changes made/needed]: None
Attachment #8880119 - Attachment is obsolete: true
Attachment #8880120 - Attachment is obsolete: true
Attachment #8880121 - Attachment is obsolete: true
Attachment #8881839 - Attachment is obsolete: true
Attachment #8885273 - Flags: approval-mozilla-beta?
Comment on attachment 8885273 [details] [diff] [review]
Squashed patch for uplift to Beta.

Fix for issues with WebExtensions and Bookmarks APIs. 
Let's land this for beta 9.
Attachment #8885273 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Bob Silverberg [:bsilverberg] from comment #28)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: No
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Bob's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
https://hg.mozilla.org/projects/jamun/rev/49c08ff2942bb6120f1f3ba8b6860c13981867e8
Bug 1374237 - Declare all top-level functions in WebExtensions API files via const. r=mixedpuppy, a=lizzard

Updated

10 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.