Closed
Bug 1328013
Opened 7 years ago
Closed 7 years ago
Option "Disable JavaScript" isn't forgotten if the tab is closed
Categories
(Firefox :: Session Restore, defect, P3)
Firefox
Session Restore
Tracking
()
VERIFIED
FIXED
Firefox 56
People
(Reporter: arni2033, Assigned: wiwang)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
3.79 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
>>> My Info: Win7_64, Nightly 49, 32bit, ID 20160526082509 STR_1: 1. Open url [1] in a new tab 2. Open devtools -> options, enable "Disable JavaScript" 3. Close tab 4. Restore tab AR: JavaScript is disabled ER: JavaScript should be enabled > [1] data:text/html,<body id="B"><script>B.style.background='gray'</script> Note: 1) Because tooltip of "Disable JavaScript" checkbox says "If the tab or the toolbox is closed, then this setting will be forgotten" 2) Because that's how it worked initially, before the regression 3) Because checkbox "Disable JavaScript" breaks after Step 4 4) This may be related to "session restore" component, because JavaScript state (enabled/disabled) is saved in session store.
My impression is that this is not a regression, but please re-set if you do find a meaningful window.
Keywords: regression
Priority: -- → P3
Comment 2•7 years ago
|
||
>>> My Info: Win7_64, Nightly 29, 32bit, ID 20140107030202 (2014-01-07) Not reproducible (good) >>> My Info: Win7_64, Nightly 53, 32bit, ID 20170107030205 (2017-01-07) Reproducible (bad) I'm not sure whether this means that it's a regression, but it seems to be one.
Comment 3•7 years ago
|
||
Here is the regression window: Last good revision: e7a366c1036c (2014-01-07) First bad revision: cf2d1bd796ea (2014-01-08) Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e7a366c1036c&tochange=cf2d1bd796ea
Comment 4•7 years ago
|
||
Regression window: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=a58fd917dead&tochange=7d4182792214 Suspect: one of the followings 3f6e82f8f9f8 Tim Taubert — Bug 952998 - Use FrameTree to collect DOMSessionStorage data r=yoric 8cbf45406b30 Tim Taubert — Bug 952934 - Use onFrameTreeReset() to re-collect docShell capability data r=smacleod be6ad59e25f5 Tim Taubert — Bug 956724 - Fix broken onload handler for DOMWindows r=rnewman
Updated•7 years ago
|
Comment 5•7 years ago
|
||
Sorry, I don't work on this code anymore.
Flags: needinfo?(ttaubert) → needinfo?(mdeboer)
Comment 6•7 years ago
|
||
I'm pretty sure that none of the changesets/ bugs listed here caused this regression. They're pointing at the work that was done to greatly improve sessionstore, which is probably why it's working too well at the moment. If I understand correctly, sessionstore should stop saving this state flag so that restored tabs will always have Javascript enabled. Right?
Flags: needinfo?(mdeboer) → needinfo?(jryans)
It's hard for me to decide what the right fix is here... Most of these page options in the DevTools toolbox try to follow the contract that everything is reset when the toolbox is closed, and they say so in the option, such as "Disable HTTP cache (when the toolbox is open)" and "Enable Service Workers over HTTP (when the toolbox is open)". "Disable JavaScript" is marked separately and says "current session only", but I am not sure what the intended meaning of "session" is here... It would probably be less surprising at least if "Disable JavaScript" is reset when the toolbox closes, and it it does work when the toolbox is closed first (separately from the tab). So, we could either: A. Make DevTools try to reset document settings during tab close B. Have session store ignore this state, like Mike mentions Mike, can you think of use cases outside of DevTools where users would want session restore to accurately restore disabled JS as it does today? Is there any browser UI outside of DevTools that even allows disabling JS per tab?
Flags: needinfo?(jryans) → needinfo?(mdeboer)
Comment 8•7 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7) > Mike, can you think of use cases outside of DevTools where users would want > session restore to accurately restore disabled JS as it does today? No. I think not persisting this state in sessionstore is a fine move. > Is there any browser UI outside of DevTools that even allows disabling JS > per tab? No. This has been removed from browser UIs explicitly.
Flags: needinfo?(mdeboer)
Comment 9•7 years ago
|
||
Will, do you have time to look at this bug to make sure we're not persisting the `allowJavascript` docShell flag? Basically it's about filtering out this specific capability from `DocShellCapabilities.jsm`. Someone from the devtools team can also work on it - it's a good-first-sessionstore-bug ;-)
Flags: needinfo?(wiwang)
Assignee | ||
Comment 10•7 years ago
|
||
Hi Mike, Sure! I can work on this, thanks :-)
Assignee: nobody → wiwang
Flags: needinfo?(wiwang)
Component: Developer Tools → Session Restore
Assignee | ||
Comment 11•7 years ago
|
||
Hi Mike, Thanks for your guide! It's done, here is the patch: - Stop collect the flag of `allowJavascript` DocShell capabilities. - eslint passed. I checked the session restore file(recovery.js) and log during development, and no any `allowJavascript` was saved/collected now. Could you please help to review? Thanks a lot!
Attachment #8853378 -
Flags: review?(mdeboer)
Comment 12•7 years ago
|
||
Comment on attachment 8853378 [details] [diff] [review] Patch(v1): Bug 1328013 - Do not collect the flag of `allowJavascript` DocShell capabilities. Review of attachment 8853378 [details] [diff] [review]: ----------------------------------------------------------------- Perhaps it'd be cool to maybe start using ReviewBoard for your patches? It's all the craze, nowadays! ::: browser/components/sessionstore/DocShellCapabilities.jsm @@ +38,5 @@ > }, > > collect(docShell) { > let caps = this.allCapabilities(docShell); > + let capsFiltered = caps.filter(cap => !docShell["allow" + cap]); Please implement this following these steps: 1. Define a constant at the top, like `const CAPABILITIES_TO_IGNORE = new Set(["Javascript"]);`, 2. Use that set in the `filter` method here, 3. Remove the obsolete code below, 4. Add a test in browser_capabilities.js to check we never store the value for realz. Thanks!
Attachment #8853378 -
Flags: review?(mdeboer) → review-
Assignee | ||
Comment 13•7 years ago
|
||
Hi Mike, Thanks for your advice! Here is the revised patch, could you help to review? (Since MozReview will be placed, allow me to use the original one here? :) )
Attachment #8853378 -
Attachment is obsolete: true
Attachment #8884766 -
Flags: review?(mdeboer)
Assignee | ||
Comment 14•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5acbcc532d47
Comment 15•7 years ago
|
||
Comment on attachment 8884766 [details] [diff] [review] Patch(v2): Bug 1328013 - Do not collect the flag of `allowJavascript` DocShell capabilities. Review of attachment 8884766 [details] [diff] [review]: ----------------------------------------------------------------- Yay! Thanks Will, nice change. ;-)
Attachment #8884766 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 16•7 years ago
|
||
Thank you for reviewing this belated patch, Mike!
Comment 18•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/85c4d1a6ad09 Do not collect the flag of `allowJavascript` DocShell capabilities. r=mikedeboer
Keywords: checkin-needed
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/85c4d1a6ad09
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Comment 20•7 years ago
|
||
I have reproduced this bug with Nightly 53.0a1 (2017-01-02) (64-bit) on Ubuntu 16.04 LTS! This bug's fix is now verified with latest Nightly! Build ID 20170717100212 User Agent Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
QA Whiteboard: [bugday-20170712]
Comment 21•7 years ago
|
||
I have successfully reproduced this bug with Nightly 53.0a1 (2017-01-01) (32-bit) on windows 10(32bit) this bug is verified fix with latest nightly 56.0a1 (2017-07-17) (32-bit) Build ID: 20170717030207 Mozilla/5.0 (Windows NT 10.0; rv:56.0) Gecko/20100101 Firefox/56.0 [bugday-20170712]
Updated•7 years ago
|
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Updated•7 years ago
|
Flags: qe-verify+
Comment 22•7 years ago
|
||
I managed to reproduce this bug both on Windows 10 x64 and Ubuntu 14.04 x32 on old versions of Nightly (2017-01-01). I retested everything using the Latest Nightly 57 and Beta 56.0b9 on Windows 10 x64, Mac OS X 10.11 and on Ubuntu 14.04 x32, the bug is not reproducing anymore. When you restore the session reloads with no problem and JavaScript is enabled.
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•