Closed Bug 1534431 Opened 7 months ago Closed 4 months ago

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

Categories

(Core :: DOM: Web Storage, defect, P1)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- disabled
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: jimb, Assigned: janv)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p2:responsiveness])

Attachments

(3 files, 1 obsolete file)

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

Blocks: 1517090
Component: General → DOM: Web Storage
Product: Firefox → Core

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.

Assignee: nobody → jvarga
Priority: -- → P1

(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.)

Status: NEW → ASSIGNED
No longer blocks: 1517090
Blocks: 1540402
Duplicate of this bug: 1539316

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

Duplicate of this bug: 1534833

I'm finishing a patch for this.

Looking forward to giving it a test...

Whiteboard: [qf:p2:responsiveness]

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.

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.

Attachment #9069354 - Attachment is obsolete: true

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

Duplicate of this bug: 1554321

(In reply to howaboutsynergy from comment #11)

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

Thanks for testing 👍!

Pushed by jvarga@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Do we need to uplift this?

Flags: needinfo?(jvarga)

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.

Flags: needinfo?(jvarga)

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
Attachment #9069353 - Flags: approval-mozilla-beta?
Attachment #9069569 - Flags: approval-mozilla-beta?

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

Attachment #9069353 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9069569 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Regressions: 1557233
You need to log in before you can comment on or make changes to this bug.