Status

()

Core
JavaScript Engine: JIT
P1
normal
RESOLVED FIXED
a month ago
23 days ago

People

(Reporter: yuyin, Assigned: yuyin)

Tracking

57 Branch
mozilla58
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a month ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/604.1.38 (KHTML, like Gecko) Version/11.0 Safari/604.1.38

Steps to reproduce:

run jit-test


Actual results:

some tests failed, for example asmjs/testCaching.js


Expected results:

tests should pass
(Assignee)

Comment 1

a month ago
 when serialize wasm code, it will call function "StoreAsmJSModuleInCache"( AsmJS.cpp), and it mprotect a write only memory.

 when serialize, it call function "StaticallyUnlink" to unpatch symbolic address, on mips platform, symbolic address is encode in the instructions('lui' 'ori' ...), unlatch symbolic address should first read the instructions to get the symbolic address and check if it is the right address, so the memory need readable.

I have push a patch to let the memory readable, if you have better idea, please let me know, thank you.
(Assignee)

Comment 2

a month ago
Created attachment 8921356 [details] [diff] [review]
0001-MIPS-Memory-of-asmjs-cache-need-readable.patch
Attachment #8921356 - Flags: review?(luke)
(Assignee)

Updated

a month ago
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
Version: 55 Branch → 57 Branch

Updated

a month ago
Assignee: nobody → yuyin-hf
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 3

a month ago
Ah, makes sense.  Instead of adding #ifdefs to only make MIPS R+W, let's do it for all platforms.
status-firefox58: --- → affected
Priority: -- → P1
status-firefox57: --- → wontfix
(Assignee)

Comment 4

a month ago
Created attachment 8921719 [details] [diff] [review]
0001-Asmjs-cache-memory-need-readable.patch
Attachment #8921719 - Flags: review?(luke)

Comment 5

a month ago
Since this changes jsapi.h, you'll also need to fix the uses in the browser:
  http://searchfox.org/mozilla-central/search?q=SetAsmJSCacheOps

Updated

a month ago
Attachment #8921356 - Attachment is obsolete: true
Attachment #8921356 - Flags: review?(luke)

Updated

a month ago
Attachment #8921719 - Flags: review?(luke)
(Assignee)

Comment 6

26 days ago
thank you for review, yes I change the function name in jsapi.h.

in fact, 
http://searchfox.org/mozilla-central/source/dom/asmjscache/AsmJSCache.cpp#280
though it use name 'OpenEntryForWrite', but it use 'READWRITE'  flag to map memory. So it does not need change anything and I did not change the function name there.

maybe I just need to do this:

-------------------------

 {
     if (!jsCachingEnabled || !jsCacheAsmJSPath)
         return JS::AsmJSCache_Disabled_ShellFlags;
@@ -7816,7 +7817,7 @@ ShellOpenAsmJSCacheEntryForWrite(HandleObject global, const char16_t* begin,
     if (memory == MAP_FAILED)
         return JS::AsmJSCache_InternalError;
     MOZ_ASSERT(*(uint32_t*)memory == 0);
-    if (mprotect(memory, serializedSize, PROT_WRITE))
+    if (mprotect(memory, serializedSize, PROT_WRITE | PROT_READ))
-----------------------------------------------

and keep the original function name ‘ShellOpenAsmJSCacheEntryForWrite’?



(In reply to Luke Wagner [:luke] from comment #5)
> Since this changes jsapi.h, you'll also need to fix the uses in the browser:
>   http://searchfox.org/mozilla-central/search?q=SetAsmJSCacheOps

Comment 7

25 days ago
(In reply to yuyin from comment #6)
Yes, just making that one tiny change makes sense to me.
(Assignee)

Comment 8

25 days ago
Created attachment 8923656 [details] [diff] [review]
0001-Asmjs-cache-memory-need-readable.patch
Attachment #8921719 - Attachment is obsolete: true
Attachment #8923656 - Flags: review?(luke)
(Assignee)

Comment 9

25 days ago
ok, thank you, attached.

(In reply to Luke Wagner [:luke] from comment #7)
> (In reply to yuyin from comment #6)
> Yes, just making that one tiny change makes sense to me.

Updated

24 days ago
Attachment #8923656 - Flags: review?(luke) → review+
(Assignee)

Updated

24 days ago
Keywords: checkin-needed

Comment 10

23 days ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d99a96c8da92
Asmjs cache memory need readable. r=luke
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d99a96c8da92
Status: ASSIGNED → RESOLVED
Last Resolved: 23 days ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.