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)

defect

Tracking

()

VERIFIED FIXED
Firefox 56
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- wontfix
firefox56 --- verified

People

(Reporter: arni2033, Assigned: wiwang)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

>>>   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.
No longer blocks: 1277113
Component: Untriaged → Developer Tools
My impression is that this is not a regression, but please re-set if you do find a meaningful window.
Keywords: regression
Priority: -- → P3
>>>   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.
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
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
Blocks: 956724
Flags: needinfo?(ttaubert)
Blocks: 952934
No longer blocks: 956724
Sorry, I don't work on this code anymore.
Flags: needinfo?(ttaubert) → needinfo?(mdeboer)
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)
(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)
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)
Hi Mike,

Sure! 
I can work on this, thanks :-)
Assignee: nobody → wiwang
Flags: needinfo?(wiwang)
Component: Developer Tools → Session Restore
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 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-
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)
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+
Thank you for reviewing this belated patch, Mike!
Please help to land the patch, thanks! :)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/85c4d1a6ad09
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
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]
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]
Status: RESOLVED → VERIFIED
Flags: qe-verify+
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.

Attachment

General

Created:
Updated:
Size: