Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

`will-navigate` event not fired when in sandbox mode #8841

Open
sebastianseilund opened this issue Mar 6, 2017 · 22 comments
Open

`will-navigate` event not fired when in sandbox mode #8841

sebastianseilund opened this issue Mar 6, 2017 · 22 comments

Comments

@sebastianseilund
Copy link

@sebastianseilund sebastianseilund commented Mar 6, 2017

Thanks for Electron! Amazing project :)

  • Electron version: 1.6.2
  • Operating system: macOS Sierra (10.12.3)

It happens when BrowserWindow is opened with sandbox: true. It might be on purpose, but I couldn't find any Electron/Chromium documentation that explains why. So I'm suspecting it's a bug. I can submit a PR with a failing test to this repo if you agree that it's a bug.

Expected behavior

webContents' will-navigate event should fire when user reloads page fx.

Actual behavior

It does not fire.

How to reproduce

package.json:

{
  "name": "electron-sandbox-will-navigate-bug",
  "version": "0.0.0",
  "main": "main.js",
  "dependencies": {
    "electron": "^1.6.2"
  }
}

main.js:

const {app, BrowserWindow} = require('electron')
const path = require('path')

app.on('ready', function() {
  const win = new BrowserWindow({
    webPreferences: {
      sandbox: true
    }
  })

  const {webContents} = win
  webContents.on('will-navigate', () => {
    console.log('will-navigate')
  })
  webContents.on('did-navigate', () => {
    console.log('did-navigate')
  })

  win.loadURL('file://' + path.join(__dirname, 'index.html'))
})

index.html:

<!doctype html>
<html>
  <body>
    <button onclick="window.location.reload()">Reload</button>
  </body>
</html>

Perform these steps:

  • Run npm install
  • Run ./node_modules/.bin/electron .
  • Click the Reload button in the browser window
  • Observe the Terminal output

Expected output:

did-navigate
will-navigate
did-navigate

Actual output:

did-navigate
did-navigate

(will-navigate is not fired).

If you outcomment the line sandbox:true in main.js, then it works as expected.

@lachenmayer

This comment has been minimized.

Copy link

@lachenmayer lachenmayer commented Mar 14, 2017

Hi there, I'm experiencing a similar issue: when the user presses cmd-R to reload, I expect the will-navigate event to fire. It currently does not.

This happens even when the sandbox: true flag is removed in the repro case above.

@jviotti

This comment has been minimized.

Copy link
Member

@jviotti jviotti commented Mar 24, 2017

This happens in OS X only for me, without sandbox.

jviotti pushed a commit to balena-io/etcher that referenced this issue Mar 24, 2017
Juan Cruz Viotti
Users can currently force the browser window to reload by using Cmd+R in
macOS. The `will-navigate` even handler in this same file is supposed to
protect us from this case, however its not fired at all just in macOS
for some reason.

As a solution, we attach dummy keyboard shortcut handlers for Cmd+R,
Ctrl+R, and F5, to override the normal browser window behaviour.

The reason behind disallowing reloads is that a reload during a critical
operation such as writing, validation, elevating, unmounting, drive
scanning, etc will cause undefined behaviour, resulting in non sense
errors that we can't solve coming to TrackJS.

See: electron/electron#8841
Fixes: #1210
Signed-off-by: Juan Cruz Viotti <jviotti@openmailbox.org>
@lurch

This comment has been minimized.

Copy link
Contributor

@lurch lurch commented Mar 25, 2017

https://github.com/resin-io/etcher/pull/1219/files implements a workaround for this.

@sebastianseilund

This comment has been minimized.

Copy link
Author

@sebastianseilund sebastianseilund commented Mar 25, 2017

https://github.com/resin-io/etcher/pull/1219/files implements a workaround for this.

Thanks for your input. I just want to point out that this is not a workaround for this specific issue. The issue here is that will-navigate is not fired. Not that I want to block the user from navigating.

@lurch

This comment has been minimized.

Copy link
Contributor

@lurch lurch commented Mar 26, 2017

Lol, whoops ;-) My apologies for misunderstanding.

jviotti pushed a commit to balena-io/etcher that referenced this issue Mar 26, 2017
Juan Cruz Viotti
Users can currently force the browser window to reload by using Cmd+R in
macOS. The `will-navigate` event handler in this same file is supposed
to protect us from this case, however it's not fired at all just in
macOS for some reason.

As a solution, we attach dummy keyboard shortcut handlers for Cmd+R,
Ctrl+R, and F5, to override the normal browser window behaviour.

The reason behind disallowing reloads is that a reload during a critical
operation such as writing, validation, elevating, unmounting, drive
scanning, etc will cause undefined behaviour, resulting in nonsense
errors that we can't solve coming to TrackJS.

See: electron/electron#8841
Fixes: #1210
Signed-off-by: Juan Cruz Viotti <jviotti@openmailbox.org>
jviotti pushed a commit to balena-io/etcher that referenced this issue Mar 26, 2017
Users can currently force the browser window to reload by using Cmd+R in
macOS. The `will-navigate` event handler in this same file is supposed
to protect us from this case, however it's not fired at all just in
macOS for some reason.

As a solution, we attach dummy keyboard shortcut handlers for Cmd+R,
Ctrl+R, and F5, to override the normal browser window behaviour.

The reason behind disallowing reloads is that a reload during a critical
operation such as writing, validation, elevating, unmounting, drive
scanning, etc will cause undefined behaviour, resulting in nonsense
errors that we can't solve coming to TrackJS.

See: electron/electron#8841
Fixes: #1210
Signed-off-by: Juan Cruz Viotti <jviotti@openmailbox.org>
@lneir

This comment has been minimized.

Copy link

@lneir lneir commented May 24, 2017

what is the status on a fix here?

@petemud

This comment has been minimized.

Copy link

@petemud petemud commented Feb 8, 2018

Actually will-navigate isn't working under linux (ubuntu 16.04) too

@codebytere

This comment has been minimized.

Copy link
Member

@codebytere codebytere commented Sep 24, 2018

We are no longer implementing bugfixes for versions of Electron <= 1.7.x, so i'm going to close this issue but if it is still persisting in more recent versions of Electron we can certainly reopen it!

@codebytere codebytere closed this Sep 24, 2018
@GhostlyDark

This comment has been minimized.

Copy link
Contributor

@GhostlyDark GhostlyDark commented Sep 24, 2018

@codebytere This bug still exists in v3.0.0, so please reopen the issue.

@codebytere

This comment has been minimized.

Copy link
Member

@codebytere codebytere commented Sep 24, 2018

Great! will do, thanks for the fast response

@codebytere codebytere reopened this Sep 24, 2018
@codebytere codebytere added 3-0-x and removed version/1.6.x labels Sep 24, 2018
dbkr added a commit to matrix-org/matrix-react-sdk that referenced this issue Jan 24, 2019
This is now more of a problem in the Electron app because of
electron/electron#8841 but is still
annoying in the webapp if you're taken away from your chat client.

Exception is the SSO link, as commented (issue filed at
vector-im/riot-web#8247).

Fixes vector-im/riot-web#8226
@geakstr

This comment has been minimized.

Copy link

@geakstr geakstr commented Feb 15, 2019

Still exists in 4.* Is there any workaround to make it works?

@geakstr

This comment has been minimized.

Copy link

@geakstr geakstr commented Feb 15, 2019

As well as in v5 beta

@markleutloff

This comment has been minimized.

Copy link

@markleutloff markleutloff commented Feb 22, 2019

I can reproduce this too, will-navigate is never fired, which makes aborting navigations for security reasons a hassle. Also nothing in the docs state that sandboxing will prevent the firing of this event.

berlysia added a commit to berlysia/n-air-app that referenced this issue Apr 24, 2019
sandbox: true 指定すると will-navigate イベントが発火しない問題がある
rel. electron/electron#8841
@mike-seekwell

This comment has been minimized.

Copy link

@mike-seekwell mike-seekwell commented May 13, 2019

you can "fix" this by doing the following:

electron.globalShortcut.register('CommandOrControl+R', () => {
    console.log('CommandOrControl+R is pressed')
})

but note that it disables CMD+R across all applications the user has open (e.g. they won't be able to reload a page in Chrome using CMD+R)

@mattbroussard

This comment has been minimized.

Copy link

@mattbroussard mattbroussard commented Jul 8, 2019

We still observe this on 5.0.5. Any updates?

The workaround of disabling the cmd-R shortcut is not applicable for us, as we are trying to prevent link navigations (i.e. not just page reloads) for security and user experience purposes.

@markleutloff

This comment has been minimized.

Copy link

@markleutloff markleutloff commented Aug 7, 2019

This is still a problem at:

  • Electron v6.0.0
  • Electron v6.0.1
  • Electron v7.0.0-beta.1

Any news on this?

@ridesky

This comment has been minimized.

Copy link

@ridesky ridesky commented Aug 9, 2019

same issue in 5.0.6

@codebytere

This comment has been minimized.

Copy link
Member

@codebytere codebytere commented Aug 9, 2019

No news, but i did some brief digging and it seems the sandbox codepath never invokes OpenURLForTab which is what causes this. Still working on the why and the fix.

@codebytere codebytere self-assigned this Aug 9, 2019
@sofianguy

This comment has been minimized.

Copy link
Member

@sofianguy sofianguy commented Aug 9, 2019

Thank you for taking the time to report this issue and helping to make Electron better.

The version of Electron you reported this on has been superseded by newer releases.

If you're still experiencing this issue in Electron v4.2.x or later, please add a comment specifying the version you're testing with and any other new information that a maintainer trying to reproduce the issue should know.

I'm setting the blocked/need-info label for the above reasons. This issue will be closed 7 days from now if there is no response.

Thanks in advance! Your help is appreciated.

@asherkin

This comment has been minimized.

Copy link

@asherkin asherkin commented Aug 9, 2019

There are 6+ months of comments confirming it is still present in versions post-4.2, please do not close this issue again.

@aegarbutt

This comment has been minimized.

Copy link
Contributor

@aegarbutt aegarbutt commented Aug 30, 2019

I did some more in-depth digging to understand when OpenURLFromTab was called. It appears it was only called when a new process was created. I've created a pull request that fixes the tactical issue.

@aegarbutt

This comment has been minimized.

Copy link
Contributor

@aegarbutt aegarbutt commented Aug 30, 2019

Perhaps this should rely on ShouldTransferNavigation instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.