1556646 - Clean up remaining NSPR code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Jan de Mooij [:jandem]]

A few things:

• PosixNSPR files have unused functions.
• These days NSPR is only used for JS::DeserializeWasmModule as far as I can see. We should consider either inlining this code and cutting the NSPR dependency completely or making PosixNSPR a more generic "no NSPR" replacement.

Comment 1

[Jan de Mooij [:jandem]]

Oh there's also the PR_LoadLibraryWithFlags in shell/js.cpp...

Comment 2

[Jan de Mooij [:jandem]]

Steve, can we always use dlopen or something in the JS shell instead of PR_LoadLibraryWithFlags? How much do we care about Windows support for this feature? It seems silly to depend on NSPR just for this :)

Comment 3

[Jan de Mooij [:jandem]]

The NSPR functions we care about for Wasm:

• PR_GetOpenFileInfo to get the file size.
• PR_CreateFileMap
• PR_MemMap
• PR_MemUnmap
• PR_CloseFileMap

Luke, how do you feel about adding our own version of this in js/src/util/? None of these look that complicated and it lets us avoid the NSPR spaghetti code. I don't know if we can get a native file descriptor out of PRFileDesc though... A simpler alternative may be to move MapFile into Gecko. WDYT?

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #3)

A simpler alternative may be to move MapFile into Gecko.

I posted a patch in bug 1556668, it seemed pretty simple.

Comment 5

[Luke Wagner [:luke]]

Thanks Jan!

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1556646 part 1 - Remove PosixNSPR remnants. r?sfink!

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1556646 part 2 - Rename JS_POSIX_NSPR to JS_WITHOUT_NSPR and remove --enable-posix-nspr-emulation configure flag. r?sfink!,glandium!

Stand-alone JS builds now default to without-NSPR on all platforms.

Note that the JS shell builds we do in automation pass --enable-nspr-build so they shouldn't be affected by
the JS shell changes.

Depends on D33932

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Just checking, these flags will not exactly be removed immediately, they'll just be turned into no-ops? I'd think it's for bisection reasons...

Comment 9

[Jan de Mooij [:jandem]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #8)

Just checking, these flags will not exactly be removed immediately, they'll just be turned into no-ops? I'd think it's for bisection reasons...

The patch here does remove the posix-nspr-emulation configure flag, but that's already the default on POSIX platforms so you could just leave it out when bisecting.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1528ef3cf76a
part 1 - Remove PosixNSPR remnants. r=sfink
https://hg.mozilla.org/integration/autoland/rev/b314f6c6148e
part 2 - Rename JS_POSIX_NSPR to JS_WITHOUT_NSPR and remove --enable-posix-nspr-emulation configure flag. r=sfink,glandium

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/1528ef3cf76a
https://hg.mozilla.org/mozilla-central/rev/b314f6c6148e

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1560432 - Remove unused NSPR header from vm/Time.cpp. r?sfink!

Gary noticed we can't build without NSPR on Windows due to this unused #include
inside #ifdef XP_WIN. This patch fixes the build.

Comment 13

[Phabricator Automation]

Comment on attachment 9073120 [details]
Bug 1560432 - Remove unused NSPR header from vm/Time.cpp. r?sfink!

Revision D35494 was moved to bug 1560432. Setting attachment 9073120 [details] to obsolete.