1592470 - Don't cancel content JS when "navigating" to javascript: URLs

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Simon]

Attachment: canvasbug.png

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

"javascript:void(document.title='reset');"
Create the above as a bookmark(without quotes), then find the NETENT slot game "Arcane Reel Chaos", then let the game load, then once loaded, click bookmark you just created.
URL that may show demo if outside UK: https://www.online-slot.co.uk/online-slots/netent-slots/arcane-reel-chaos/

I also have ublock origin and tampermonkey installed if there is trouble reproducing. I'm also on Windows 7.

Actual results:

The tab bar title changes... however the canvas drawing of the slot appears to crash, several "reels" of the slot seem to vanish, this game is the most consistent case I could find, however as I am from UK its hard to find a free play URL to test this on.
If you're outside of the UK, this demo may load from this URL here:
https://www.online-slot.co.uk/online-slots/netent-slots/arcane-reel-chaos/

Picture attached is from a non-free play version of the game.

Expected results:

The drawing of the canvas should be unaffected, the title bar changes to "reset" as it has done normally.

Comment 1

[Simona Badau, Desktop QA, PTO 13-25aug]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:72.0) Gecko/20100101 Firefox/72.0
Build ID: 20191104094118

I can confirm that this issue is reproducible on the latest Nightly 72.0a1, Firefox 71 beta 6 and Firefox 70.0.1 - tested on Windows 7.

Comment 2

[Simona Badau, Desktop QA, PTO 13-25aug]

Hi guys,
I managed to get a regression range for this issue, here are the results:

• last known good: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=25b1ee5ee458657bfa703ffa34237e6e99ad999e&tochange=04d9dba72f221b5808f077e3aac5522147008438

• first known bad: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=65d689fa9955206188556cc445080b3528195ddb&tochange=04d9dba72f221b5808f077e3aac5522147008438

Comment 3

[Simon]

Thanks, it seems to be connected to the setting dom.ipc.cancel_content_js_when_navigating , when set to false, the page works as expected, no canvas crash

Comment 4

[Jim Porter (:squib)]

Attachment: Bug 1592470 - Don't cancel content JS when "navigating" to javascript: URLs; r?smaug

Prior to this patch, users entering javascript: URLs in the location bar
(e.g. via a bookmark) would trigger the cancel content JS path for the page
they were on, potentially breaking the site. Since javascript: URLs
(generally) don't navigate to a new page, we should avoid canceling the
current page's JS.

Comment 5

[Simon]

Thanks for the patch, just wondering out of curiousity though, would it still cancel content js if a bookmarklet did happen to navigate away?
Say would a bookmarklet setting a different location.href would work the same way as a script tag setting a different location.href? Either way doesn't affect me btw.

Comment 6

[Jim Porter (:squib)]

(In reply to Simon from comment #5)

Thanks for the patch, just wondering out of curiousity though, would it still cancel content js if a bookmarklet did happen to navigate away?

No, it wouldn't. It ends up being a catch 22. When we load another page, part of that operation gets put into the page's event queue, so if the event loop stalls (i.e. because the page is busy running some synchronous JS), we normally have to wait for that to finish in order for our load to go through. The "cancel content JS" code works around this by examining the new URL and asking whether it will send us to a new top-level page; if so, we just force-terminate the current page's JS since we know that the page is going to go away in a second.

With this architecture, it's not easy to tell if a javascript: URL would take us to a different page, since we need to run the JS in that URL to figure it out. However, in order to run the JS, we need to perform the full "load" on that javascript: URL, putting the JS code in it into the page's event queue. That gets stalled due to the issue mentioned above. (Obviously, it's easy to tell if we'd navigate to a new page for simple javascript: URLs, but it'd be impossible to tell for more complex cases.)

Say would a bookmarklet setting a different location.href would work the same way as a script tag setting a different location.href? Either way doesn't affect me btw.

That said, this does still work the same way as a <script> tag: if a page's event queue is stalled, then a script setting location.href would end up stuck waiting in the queue, just like a bookmarklet. In both cases, you're effectively just saying, "Please run this JS when you get the chance."

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/099cebd1bbd7
Don't cancel content JS when "navigating" to javascript: URLs; r=smaug

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/099cebd1bbd7

Comment 9

[Jim Porter (:squib)]

Comment on attachment 9106763 [details]
Bug 1592470 - Don't cancel content JS when "navigating" to javascript: URLs; r?smaug

Beta/Release Uplift Approval Request

• User impact if declined: Users who use javascript: bookmarklets to modify pages may see broken pages if the bookmarklet is run while the page has active JS.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch just adds an extra conditional to prevent the "cancel content JS" logic from running if the URL to navigate to is a javascript: URL. Since this just prevents Firefox from doing something it shouldn't, there should be minimal risk of fallout.
• String changes made/needed:

Comment 10

[Jim Porter (:squib)]

To verify this is fixed, you can do the following:

1. (Optional) Open devtools and switch to the console
2. Navigate to http://mozilla.pettay.fi/moztests/goback.html
3. Wait for it to redirect to goback2.html (which contains an infinite JS loop to stall the event queue)
4. Enter javascript:void(0) in the location bar and hit Enter
5. Hit the back button: you should not go back to the previous page
6. (Optional) Type in any URL you like in the location bar and hit Enter: you should go to that page

If you're checking the devtools console output, this is how it should behave: at the end of step (4), you should not see a message like the following. However, you should see it after step (6):

Error: Script terminated by timeout at:
@http://mozilla.pettay.fi/moztests/goback2.html:13:13
goback2.html:13:13

Comment 11

[Pascal Chevrel:pascalc (PTO until Sept 2)]

Comment on attachment 9106763 [details]
Bug 1592470 - Don't cancel content JS when "navigating" to javascript: URLs; r?smaug

The patch is minimal and has tests, let's have QA verify the fix as well, uplift approved for 71 beta 10, thanks.

Comment 12

[Narcis Beleuzu [:NarcisB]]

Merge conflicts while grafting 099cebd1bbd7

merging dom/ipc/BrowserChild.cpp
merging dom/ipc/tests/browser.ini
warning: conflicts while merging dom/ipc/tests/browser.ini! (edit, then use 'hg resolve --mark')

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/93ae6a5db1d3

Comment 14

[Simona Badau, Desktop QA, PTO 13-25aug]

Fix verified using the latest Nightly 72.0a1 (2019-11-14) on Windows 10, Ubuntu 18.04, macOS 10.15.

Comment 15

[Daniel Cicas [:dcicas], Release QA]

Hello,

I can confirm that this issue is fixed on Fx 71.0b10 on Windows 10 x64, macOS 10.13 and Ubuntu 18.04.