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)
Tracking
(firefox55 fixed, firefox56 fixed)
RESOLVED
FIXED
mozilla56
People
(Reporter: svanderger, Assigned: bsilverberg)
Details
Attachments
(4 files, 4 obsolete files)
13.47 KB,
application/x-zip-compressed
|
Details | |
182.36 KB,
image/png
|
Details | |
182.73 KB,
image/png
|
Details | |
64.65 KB,
patch
|
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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"
Updated•7 years ago
|
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"
Updated•7 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"
Updated•7 years ago
|
Flags: needinfo?(amckay)
Comment 3•7 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•7 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•7 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•7 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•7 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)
Comment 8•7 years ago
|
||
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•7 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)
Comment 10•7 years ago
|
||
(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•7 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•7 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•7 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•7 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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 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•7 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•7 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
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dd6ecbda6ab2 https://hg.mozilla.org/mozilla-central/rev/20b6ab269217 https://hg.mozilla.org/mozilla-central/rev/6fe1b354f47f https://hg.mozilla.org/mozilla-central/rev/019d09aaa261
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee | ||
Comment 27•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=287613aabdaa7202eb94325d46e8a470bc5ec374
Assignee | ||
Comment 28•7 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?
Updated•7 years ago
|
status-firefox55:
--- → affected
Comment 29•7 years ago
|
||
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+
Comment 30•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/49c08ff2942b
Comment 31•7 years ago
|
||
(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-
Comment 32•7 years ago
|
||
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•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•