1646560 - Fix GetInProcessParentDocshell usage in nsDocShell::RecomputeCanExecuteScripts

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Kris Maglione [:kmag]]

It ignores OOP ancestors, which it probably shouldn't. The flag probably needs to move to BrowsingContext instead.

Comment 1

[Neha Kochar [:neha]]

Used for disabling JS for a page, probably used by devtools.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

kmag, I have a few spare cycles, so Neha suggested I could pick this bug, but I don't quite understand the objective. Are there any tests I can run to get a better idea of the problem?

Comment 3

[Kris Maglione [:kmag]]

I'm not sure. Devtools definitely toggles the allowJavascript flag, and would therefore be affected by this:

https://searchfox.org/mozilla-central/source/devtools/server/actors/targets/browsing-context.js#1251

But I don't know what tests would use it, or whether there are any existing tests that use it with OOP subframes.

Either way, I think we probably just want to move the flag from nsDocShell to BrowsingContext, and ideally change Devtools to set it in the parent so we can forbid trying to set it from a child.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

So, to summarize:

1. Move allowJavascript from nsDocShell to BrowsingContext.
2. Fix something with RecomputeCanExecuteScript. I don't understand what yet. Remove the dependency on parent->mCanExecuteScripts, maybe?
3. Fix DevTools so that it touches allowJavascript only in the parent.
4. Remove/forbid allowJavascript in the child.

Do I understand correctly so far?

Comment 5

[Kris Maglione [:kmag]]

RecomputeCanExecuteScript would just have to look at ancestor BrowsingContexts rather than ancestor DocShells.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1646560 - Move RecomputeCanExecuteScript from nsDocShell to BrowsingContext;r?kmag

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1646560 - Move allowJavascript from docShell to BrowsingContext (Session Restore);r?smacleod

Depends on D91197

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1646560 - Move allowJavascript from docShell to BrowsingContext (DevTools);r?ochameau

Depends on D91412

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1646560 - Move allowJavascript from docShell to BrowsingContext (GeckoView);r?agi

Depends on D91413

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Latest try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5182f0d8883702ae0aa702cb8729e32a8482b1f8

Comment 11

[David Teller [:Yoric] - still alive but not very active]

I won't have time to finish this.

Comment 12

[Neha Kochar [:neha]]

Steven, there is one patch from yoric that needs to be re-written. Can you pick this up please?

Comment 13

[Neha Kochar [:neha]]

This is only used by devtools and doesn't block 50% Fission Nightly experiment. Moving it to M7 to block Fission on Beta.
Steven, when you get to work on this, please discuss with Nika as she has thoughts on how this should be done correctly by moving to the parent process.

Comment 14

[Steven MacLeod [:smacleod]]

Attachment: Bug 1646560 - Remove SessionStore's restoring of allowJavascript. r=farre!

We stopped saving this information long ago (see Bug 1328013) but the
restoration code was left in place. Since allowJavascript is being moved
to the BrowsingContext we might as well remove the old restoration code.

Comment 15

[Steven MacLeod [:smacleod]]

(In reply to Neha Kochar [:neha] from comment #13)

This is only used by devtools and doesn't block 50% Fission Nightly experiment. Moving it to M7 to block Fission on Beta.
Steven, when you get to work on this, please discuss with Nika as she has thoughts on how this should be done correctly by moving to the parent process.

Sounds good. I've dumped my WIP patches to phabricator and will come back to this once my other M6c work is complete.

Comment 16

[Kris Maglione [:kmag]]

Attachment: Bug 1646560: Part 1 - Move some XPCShell content helpers to generic shared module. r=nika

They were originally written for the extension framework, but have been used
more and more widely ever since.

Comment 17

[Kris Maglione [:kmag]]

Attachment: Bug 1646560: Part 2 - Move allowJavascript and friends from DocShell to BrowsingContext and WindowContext. r=nika,jdescottes

This is slightly complicated by the fact that the editor code wants to be able
to set this from the content process, so we really need separate
BrowsingContext and WindowContext flags, the latter of which can be set by the
owning process.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/284dc9ca5f5c
Part 1 - Move some XPCShell content helpers to generic shared module. r=nika
https://hg.mozilla.org/integration/autoland/rev/0fa59c7f0f82
Part 2 - Move allowJavascript and friends from DocShell to BrowsingContext and WindowContext. r=jdescottes,nika,geckoview-reviewers,devtools-backward-compat-reviewers,agi

Comment 19

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/284dc9ca5f5c
https://hg.mozilla.org/mozilla-central/rev/0fa59c7f0f82

Comment 20

[Kris Maglione [:kmag]]

Attachment: Bug 1646560: Follow-up: Review comment fixes that wound up in the wrong patch.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ddb25d50aa6
Follow-up: Review comment fixes that wound up in the wrong patch.

Comment 22

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/3ddb25d50aa6