Closed Bug 1556646 Opened 6 months ago Closed 6 months ago

Clean up remaining NSPR code

Categories

(Core :: JavaScript Engine, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(2 files, 1 obsolete file)

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.

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

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 :)

Flags: needinfo?(sphink)

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?

Flags: needinfo?(luke)
Depends on: 1556668

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

Flags: needinfo?(luke)

Thanks Jan!

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

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

Flags: needinfo?(jdemooij)

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

Flags: needinfo?(jdemooij)
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(sphink)
Priority: -- → P1
Pushed by jdemooij@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

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

Regressions: 1560432
Attachment #9073120 - Attachment description: Bug 1556646 - Remove unused NSPR header from vm/Time.cpp. r?sfink! → Bug 1560432 - Remove unused NSPR header from vm/Time.cpp. r?sfink!

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.

Attachment #9073120 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.