Closed Bug 1374237 Opened 7 years ago Closed 7 years ago

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

Categories

(WebExtensions :: Frontend, defect, P1)

56 Branch
defect

Tracking

(firefox55 fixed, firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: svanderger, Assigned: bsilverberg)

Details

Attachments

(4 files, 4 obsolete files)

Attached 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
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"
Flags: needinfo?(amckay)
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
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
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
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
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)
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)
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 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 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 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)
(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 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+
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
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: