Closed
Bug 1191557
Opened 9 years ago
Closed 9 years ago
Use "inner-window-destroyed" message for shutting down content sandbox in extension API
Categories
(WebExtensions :: Untriaged, defect)
Tracking
(firefox45 fixed)
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: billm, Assigned: billm)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
4.48 KB,
patch
|
kmag
:
review+
|
Details | Diff | Splinter Review |
The use of the dom-window-destroyed notification is pretty broken since we're passed the outer window.
Assignee | ||
Updated•9 years ago
|
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
Updated•9 years ago
|
Iteration: --- → 44.2 - Oct 19
Updated•9 years ago
|
Iteration: 44.2 - Oct 19 → 44.3 - Nov 2
Updated•9 years ago
|
Flags: blocking-webextensions-
Assignee | ||
Comment 1•9 years ago
|
||
This just switches to the safer inner-window-destroyed notification. It's safer because we're getting a window ID rather than a window object that's in the process of being destroyed, which I've seen cause issues. I wasn't able to write a test for this unfortunately.
Attachment #8680932 -
Flags: review?(kmaglione+bmo)
Comment 2•9 years ago
|
||
Comment on attachment 8680932 [details] [diff] [review] patch Review of attachment 8680932 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/extensions/ExtensionContent.jsm @@ +311,5 @@ > if (!mm || !ExtensionContent.globals.has(mm)) { > return; > } > > + this.windows.delete(windowId(window)); This doesn't seem right. Are we expecting to have an entry for the window at this point? I could see how that might be expected for the outer window, but not the inner window. Either way, it would probably be a good idea to log an error, since deleting it means contexts may not be closed properly. @@ +405,5 @@ > > shutdownExtension(extensionId) { > for (let global of ExtensionContent.globals.keys()) { > for (let [window, state] of this.enumerateWindows(global.docShell)) { > + let extensions = this.windows.get(windowId(window)); What about windows hidden in the BFCache? Now that |windows| is a Map rather than a WeakMap, we should probably just iterate over its entries.
Attachment #8680932 -
Flags: review?(kmaglione+bmo) → review+
Updated•9 years ago
|
Iteration: 44.3 - Nov 2 → 45.1 - Nov 16
Comment 4•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0bec4cffcbae
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•