1534431 - Nightly hangs in local storage when following links on file:///

Status: RESOLVED FIXED

(Core :: Storage: localStorage & sessionStorage, defect, P1)

Description

[Jim Blandy :jimb]

Attachment: Backtrace of content process.

In changeset eb0a25f393aa (2019-3-11), visiting Rust documentation at a file:/// url hangs the content process.

In mach run, I visited the URL:
file:///home/jimb/.multirust/toolchains/nightly-x86_64-unknown-linux-gnu/share/doc/rust/html/index.html

This page contained a link:
<a href="std/index.html">extensive API documentation</a>

If I follow that link, then the tab hangs with the stack attached. The JavaScript stack is simply:

0 getCurrentValue(name = "rustdoc-theme") ["file:///home/jimb/.multirust/toolchains/nightly-x86_64-unknown-linux-gnu/share/doc/rust/html/storage.js":1:1157]
this = [object Window]
1 <TOP LEVEL> ["file:///home/jimb/.multirust/toolchains/nightly-x86_64-unknown-linux-gnu/share/doc/rust/html/storage.js":1:1835]

Oddly, if instead of following that link, I create a new tab and visit this URL directly, then the page loads fine:
file:///home/jimb/.multirust/toolchains/nightly-x86_64-unknown-linux-gnu/share/doc/rust/html/std/index.html

On the console, the following error message gets printed when the tab hangs:

###!!! [Parent][DispatchAsyncMessage] Error: PBackground::Msg_PBackgroundLSRequestConstructor Value error: message was deserialized, but contained an illegal value

Comment 1

[Jan Varga [:janv]]

This seems to have nothing with a11y (bug 1516136 and bug 1534208) which is good news.

Anyway, I can reproduce it and I see this warning in the terminal:
[Child 25445, Main Thread] WARNING: '!ClientIsValidCreationURL(mClientInfo.PrincipalInfo(), aArgs.url())', file /Users/varga/Sources/Mozilla0/dom/clients/manager/ClientSource.cpp, line 54

Andrew this is a similar problem as with the sessionrestore'd file:/// page.

When I load this page: file:///Users/varga/Sites/ls-file-hang-test.html
I get these origins in ClientIsValidCreationURL:
origin: file:///Users/varga/Sites/ls-file-hang-test.html
principalOrigin: file:///Users/varga/Sites/ls-file-hang-test.html
They equal and the check in LS passes.

Once I click on the link: <a href="std/index.html">extensive API documentation</a>
I get these origins in ClientIsValidCreationURL:
origin: file:///Users/varga/Sites/std/index.html
principalOrigin: file:///Users/varga/Sites/ls-file-hang-test.html
They don't equal, so the check fails in we refuse to create a parent actor.

Comment 2

[Jim Blandy :jimb]

(In reply to Jan Varga [:janv] from comment #1)

Andrew this is a similar problem as with the sessionrestore'd file:/// page.

In fact, that's how I first encountered this: restarting Firefox after a rebuild, my pinned tabs for Rust documentation would hang (and all other file:/// URLs with them, I guess because of process grouping.)

Comment 4

[Jim Blandy :jimb]

I can still reproduce this in 30817917ace8 (Tue May 14).

Comment 6

[Jan Varga [:janv]]

I'm finishing a patch for this.

Comment 7

[u636583]

Looking forward to giving it a test...

Comment 8

[Jan Varga [:janv]]

Attachment: Bug 1534431 - Part 1: Verify LS request parameters as part of LS request state machine; r=asuth

LS Request parameters are now verified as part of the state machine so an actor
is always created. Before this patch, we were doing verification prior to actor
creation. If the verification failed, we didn't create an actor and content
was endlessly waiting for a reply.

Comment 9

[Jan Varga [:janv]]

Attachment: Big 1534431 - Part 2: Disallow LS requests with undefined client id; r=asuth

Comment 10

[Jan Varga [:janv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5385563a12014d52cd42edfaff89d30c9e132f6e

Comment 11

[u636583]

tl;dr: patches worked! Issue is no more.

used url: file:///home/user/build/2nonpkgs/rust.stuff/rust/rust.installed.dir/share/doc/rust/html/book/ch19-01-unsafe-rust.html#dereferencing-a-raw-pointer

tested local/firefox-hg r476510+.c143aa387e91+-1 without patches to confirm the issue still exists (with dom.storage.next_gen=true, of course) - hangs even when changing the theme to Coal.

tested local/firefox-hg r476594+.dfc513e7e141+-1 with both patches on top and I can confirm that the issue is gone.

So now even changing the theme(to Coal) is snappy.

Thank you for your work.

Comment 12

[Jan Varga [:janv]]

Attachment: Bug 1534431 - Part 2: Disallow LS requests with undefined client id; r=asuth

Comment 13

[Jan Varga [:janv]]

Glad to hear it works for you, thanks for testing!

Comment 15

[Andrew Overholt [:overholt]]

(In reply to howaboutsynergy from comment #11)

tl;dr: patches worked! Issue is no more.

Thanks for testing 👍!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c35a43b830c
Part 1: Verify LS request parameters as part of LS request state machine; r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fcdc0352853
Part 2: Disallow LS requests with undefined client id; r=asuth

Comment 17

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/4c35a43b830c
https://hg.mozilla.org/mozilla-central/rev/7fcdc0352853

Comment 18

[Julien Cristau [:jcristau]]

Do we need to uplift this?

Comment 19

[Jan Varga [:janv]]

Yes, the patches are relatively big, but most of the changes are just moving of code to a different place. Real changes are subtle.
I'll request beta-approvals if it's ok.

Comment 20

[Jan Varga [:janv]]

Comment on attachment 9069353 [details]
Bug 1534431 - Part 1: Verify LS request parameters as part of LS request state machine; r=asuth

Beta/Release Uplift Approval Request

• User impact if declined: Users experience hangs when a session restored file:// page accesses LocalStorage.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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): Most of the changes are moving of code to a different place. Patches landed 5 days ago. No regressions reported.
• String changes made/needed: None

Comment 22

[Julien Cristau [:jcristau]]

Comment on attachment 9069353 [details]
Bug 1534431 - Part 1: Verify LS request parameters as part of LS request state machine; r=asuth

lsng fix for 68.0b10

Comment 23

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e9a8bcc18342
https://hg.mozilla.org/releases/mozilla-beta/rev/5bc174324f87