Closed
Bug 1153978
Opened 9 years ago
Closed 8 years ago
Use the build id instead of the XDR_BYTECODE_VERSION constant
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jandem, Assigned: arai)
References
Details
Attachments
(5 files, 4 obsolete files)
4.93 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
13.96 KB,
patch
|
jandem
:
review+
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
13.14 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
1.93 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
2.43 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
The XDR version is stored as a constant that has to be incremented whenever the bytecode (or error messages) change. This is a bit tedious, easy to forget and it can be pretty annoying when you get merge conflicts (especially when uplifting patches..) The asm.js cache uses the browser's build id (via a callback), I think it'd be pretty simple and nice to reuse this mechanism for XDR. The only caveat is that you might run into problems when you do a local build and rebuild only xul (mach build js/src) and not the Firefox executable. Personally I don't think it's worth keeping the current mechanism just for that use case, also because, in my experience, most patches that bump this constant are only tested in the shell.
Comment 1•9 years ago
|
||
What would be the impact on nightly users?
Reporter | ||
Comment 2•9 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #1) > What would be the impact on nightly users? Their XDR caches will be invalidated each time they update, but that's fine I think.
Comment 3•8 years ago
|
||
Ted: Do you know if using build-id, extracted like so: http://mxr.mozilla.org/mozilla-central/source/dom/asmjscache/AsmJSCache.cpp#1698 as the way to invalidate our XDR bytecode cache (which isn't CPU/arch dependent) is sufficient as a replacement for the current XDR number we bump manually: http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Xdr.h#18 ? (Sorry for shotgun ni; feel free to forward if you can think of a better person.)
Flags: needinfo?(ted)
Assignee | ||
Comment 4•8 years ago
|
||
Here's draft patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a7719e29155 changes: Part 1 * Added JS::SetBuildIdOp public JSAPI * Moved buildId member from JS::AsmJSCacheOps to JSRuntime * Added ExclusiveContext::buildId method to access runtime's buildId * Moved GetBuildId to xpcom/base/CycleCollectedJSRuntime.cpp * Use GRE_BUILDID constant directly instead of nsIXULAppInfo * nsIXULAppInfo is not available on xpcshell, that is used while packaging * Added xpcom/base/Makefile.in to define GRE_BUILDID * Call JS::SetBuildIdOp in CycleCollectedJSRuntime::CycleCollectedJSRuntime * Call JS::SetBuildIdOp in main in js shell Part 2 * Changed XDR VersionCheck to use cx->buildId() * encode/decode length(uint32) + buildId(bytes) current XDR_BYTECODE_VERSION(uint32) will be decoded as length, and it's rejected because of different length, that is checked before decoding buildId bytes * Added GetBuildId function to js/src/jsapi-tests/testXDR.cpp * Call JS::SetBuildIdOp in FreezeThaw in js/src/jsapi-tests/testXDR.cpp * Removed XDR_BYTECODE_VERSION, XDR_BYTECODE_VERSION_SUBTRAHEND, and JSErr_Limit check \o/ * Removed XDR_BYTECODE_VERSION-related code from make_opcode_doc.py * Removed XDR_BYTECODE_VERSION-related comment from SourceNotes.h * Removed XDR_BYTECODE_VERSION-related comment from jsfriendapi.h then, I have some questions: 1. As noted above, nsIXULAppInfo is not available in xpcshell, is it okay to embed GRE_BUILDID directly in CycleCollectedJSRuntime.cpp ? https://dxr.mozilla.org/mozilla-central/rev/c5cb194cc9cb56d742fb3a7a826f0080b0404edc/toolkit/xre/nsAppRunner.cpp#803 > NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIXULAppInfo, gAppData || > XRE_IsContentProcess()) there, gAppData is null, and XRE_IsContentProcess() is false on xpcshell 2. Now buildId is used in XDR encode/decode, and all runtime that may use XDR needs JS::SetBuildIdOp call. Where/when is the best place/timing to call it? Looks like CycleCollectedJSRuntime is used for most of runtimes in firefox/xpcshell, so I added it there for now. Is there any better place?
Assignee | ||
Comment 5•8 years ago
|
||
Can I get some feedback?
Attachment #8703245 -
Flags: feedback?(jdemooij)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8703246 -
Flags: feedback?(jdemooij)
Reporter | ||
Comment 7•8 years ago
|
||
Comment on attachment 8703245 [details] [diff] [review] Part 1: Separate buildIdOp from AsmJSCacheOps. Review of attachment 8703245 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks! ::: js/src/asmjs/AsmJSModule.cpp @@ +1990,5 @@ > JS::BuildIdCharVector buildId_; > > public: > bool extractCurrentState(ExclusiveContext* cx) { > + if (!cx->buildId()) Nit: I'd call this buildIdOp() to make it more clear it returns a callback and not the buildId itself. That also nicely matches AsmJSCacheOps etc. ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +401,5 @@ > +mozilla::GetBuildId(JS::BuildIdCharVector* aBuildID) > +{ > +#ifndef GRE_BUILDID > +# error "GRE_BUILDID should be defined." > +#endif Nice. ::: xpcom/base/Makefile.in @@ +1,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +include $(topsrcdir)/config/makefiles/makeutils.mk You'll need review from a build peer (like glandium or ted) for this change :)
Attachment #8703245 -
Flags: feedback?(jdemooij) → feedback+
Reporter | ||
Comment 8•8 years ago
|
||
Comment on attachment 8703246 [details] [diff] [review] Part 2: Use the build id instead of the XDR_BYTECODE_VERSION constant. Review of attachment 8703246 [details] [diff] [review]: ----------------------------------------------------------------- \o/ ::: js/src/vm/Xdr.cpp @@ +16,5 @@ > #include "vm/Debugger.h" > #include "vm/ScopeObject.h" > > using namespace js; > +using mozilla::PodEqual; Nit: either remove this line, or s/mozilla::PodEqual/PodEqual/ below :) @@ +99,5 @@ > + if (!xdr->cx()->buildId() || !xdr->cx()->buildId()(&buildId)) { > + JS_ReportErrorNumber(xdr->cx(), GetErrorMessage, nullptr, JSMSG_BUILD_ID_NOT_AVAILABLE); > + return false; > + } > + Maybe add a MOZ_ASSERT(!buildId.empty()); here, as a simple sanity check. @@ +112,1 @@ > JS_ReportErrorNumber(xdr->cx(), GetErrorMessage, nullptr, JSMSG_BAD_SCRIPT_MAGIC); We can rename JSMSG_BAD_SCRIPT_MAGIC to JSMSG_BAD_BUILD_ID or something. @@ +120,5 @@ > + JS::BuildIdCharVector decodedBuildId; > + > + /* buildIdLength is already checked against the length of current > + * buildId. */ > + if (!decodedBuildId.resize(buildIdLength)) { Nit: use // comments
Attachment #8703246 -
Flags: feedback?(jdemooij) → feedback+
Comment 9•8 years ago
|
||
Dup of 601814 ?
Assignee | ||
Comment 10•8 years ago
|
||
Thank you for your feedback :) Addressed review comments. ted, can you review the change in xpcom/base/Makefile.in? in addition to questions in comment #3 and #4.
Assignee: jdemooij → arai.unmht
Attachment #8703245 -
Attachment is obsolete: true
Attachment #8703910 -
Flags: review?(ted)
Comment 11•8 years ago
|
||
The build id is always increasing, and it will get regenerated for any top-level build, so for those purposes it should be fine. We already use it for invaliding some XUL caches etc.
Flags: needinfo?(ted)
Comment 12•8 years ago
|
||
Comment on attachment 8703910 [details] [diff] [review] Part 1: Separate buildIdOp from AsmJSCacheOps. Review of attachment 8703910 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/base/CycleCollectedJSRuntime.cpp @@ +408,5 @@ > +mozilla::GetBuildId(JS::BuildIdCharVector* aBuildID) > +{ > +#ifndef GRE_BUILDID > +# error "GRE_BUILDID should be defined." > +#endif Is there any reason you can't just grab this off of nsIXULAppInfo? https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/xpcom/system/nsIXULAppInfo.idl#56 I'm not a fan of adding more Makefile.ins at this point.
Attachment #8703910 -
Flags: review?(ted)
Assignee | ||
Comment 13•8 years ago
|
||
Thank you for reviewing :D (In reply to Ted Mielczarek [:ted.mielczarek] from comment #12) > Is there any reason you can't just grab this off of nsIXULAppInfo? > https://dxr.mozilla.org/mozilla-central/rev/ > aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/xpcom/system/nsIXULAppInfo.idl#56 nsIXULAppInfo is not available in xpcshell. XDR with xpcshell is used while packaging jsm, so I needed other way to get build id. Maybe I should make it possible to use nsIXULAppInfo in xpcshell? https://dxr.mozilla.org/mozilla-central/rev/c5cb194cc9cb56d742fb3a7a826f0080b0404edc/toolkit/xre/nsAppRunner.cpp#803 > NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIXULAppInfo, gAppData || > XRE_IsContentProcess()) gAppData is null there.
Flags: needinfo?(ted)
Comment 14•8 years ago
|
||
Oh, right, sorry. I'm still not wild about this because this will wind up re-linking libxul every time someone does a top-level rebuild (the build id gets regenerated, which will cause CycleCollectedJSRuntime.cpp to get rebuilt, which will cause libxul to relink). Making nsIXULAppInfo always available in xpcshell would break some things. The build ID gets into nsIXULAppInfo by way of being compiled into nsBrowserApp.cpp: https://dxr.mozilla.org/mozilla-central/source/browser/app/nsBrowserApp.cpp#8 We could do a similar thing for the xpcshell binary, but then you'd have to do some plumbing to make it available down in the cycle collector code.
Flags: needinfo?(ted)
Assignee | ||
Comment 15•8 years ago
|
||
thanks :) looks like gToolkitVersion is always available. so I'll prepare a dedicated way to pass it to CycleCollectedJSRuntime.cpp, maybe another interface on nsXULAppInfo, in case nsIXULAppInfo is not implemented.
Assignee | ||
Comment 16•8 years ago
|
||
Added nsIPlatformInfo interface with platformVersion and platformBuildID properties, and made nsIXULAppInfo inherits from nsIPlatformInfo. nsXULAppInfo always implements nsIPlatformInfo interface, regardless of gAppData, as platformVersion and platformBuildID are always available via gToolkitVersion and gToolkitBuildID global variables.
Attachment #8715633 -
Flags: review?(ted)
Assignee | ||
Comment 17•8 years ago
|
||
changed mozilla::GetBuildId to get platformBuildID by nsIPlatformInfo interface of app-info.
Attachment #8703910 -
Attachment is obsolete: true
Attachment #8715634 -
Flags: review?(ted)
Assignee | ||
Updated•8 years ago
|
Attachment #8715633 -
Flags: review?(ted) → review?(jst)
Assignee | ||
Comment 18•8 years ago
|
||
Attachment #8715634 -
Attachment is obsolete: true
Attachment #8715634 -
Flags: review?(ted)
Attachment #8728553 -
Flags: review?(jdemooij)
Assignee | ||
Comment 19•8 years ago
|
||
Attachment #8703246 -
Attachment is obsolete: true
Attachment #8728554 -
Flags: review?(jdemooij)
Reporter | ||
Comment 20•8 years ago
|
||
Comment on attachment 8728553 [details] [diff] [review] Part 1: Separate buildIdOp from AsmJSCacheOps. Review of attachment 8728553 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I'm not a Gecko peer though so, bz, would you mind glancing at the non-JS parts? It's just moving GetBuildId to CycleCollectedJSRuntime.cpp and setting the build id hook there instead of in nsJSEnvironment.cpp. ::: xpcom/base/CycleCollectedJSRuntime.h @@ +417,5 @@ > return aKind == JS::TraceKind::Object || aKind == JS::TraceKind::Script; > } > > +bool > +GetBuildId(JS::BuildIdCharVector* aBuildID); I think GetBuildId is only used in CycleCollectedJSRuntime.cpp. If that's true, we can remove this declaration and change CycleCollectedJSRuntime.cpp to have: static bool GetBuildId(...) {
Attachment #8728553 -
Flags: review?(jdemooij)
Attachment #8728553 -
Flags: review?(bzbarsky)
Attachment #8728553 -
Flags: review+
Reporter | ||
Comment 21•8 years ago
|
||
Comment on attachment 8728554 [details] [diff] [review] Part 2: Use the build id instead of the XDR_BYTECODE_VERSION constant. Review of attachment 8728554 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Xdr.h @@ -30,5 @@ > - * https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode > - * > - * (If you're wondering, 0xb973c0de is used because it looks like "bytecode".) > - */ > -static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 350; The end of an era :'(
Attachment #8728554 -
Flags: review?(jdemooij) → review+
Updated•8 years ago
|
Attachment #8715633 -
Flags: review?(jst) → review+
Assignee | ||
Comment 22•8 years ago
|
||
Thank you for reviewing :) (In reply to Jan de Mooij [:jandem] from comment #20) > ::: xpcom/base/CycleCollectedJSRuntime.h > @@ +417,5 @@ > > return aKind == JS::TraceKind::Object || aKind == JS::TraceKind::Script; > > } > > > > +bool > > +GetBuildId(JS::BuildIdCharVector* aBuildID); > > I think GetBuildId is only used in CycleCollectedJSRuntime.cpp. If that's > true, we can remove this declaration and change CycleCollectedJSRuntime.cpp > to have: > > static bool > GetBuildId(...) > { It's used also in AsmJSCache.cpp https://dxr.mozilla.org/mozilla-central/rev/af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86/dom/asmjscache/AsmJSCache.cpp#84 https://dxr.mozilla.org/mozilla-central/rev/af7c0cb0798f5425d5d344cbaf0ac0ecb1a72a86/dom/asmjscache/AsmJSCache.cpp#116
Comment 23•8 years ago
|
||
Comment on attachment 8728553 [details] [diff] [review] Part 1: Separate buildIdOp from AsmJSCacheOps. Yeah, just make this thing a file static, if it's only used in this file. r=me on the xpcom and dom changes with that fixed.
Attachment #8728553 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 24•8 years ago
|
||
The build ID string is now shared between XDR bytecode and asm.js cache, and GetBuildId is used in 2 files CycleCollectedJSRuntime.cpp (to call JS::SetBuildIdOp with it) and AsmJSCache.cpp (directly call it from WriteMetadataFile and ReadMetadataFile). so it cannot be made static. am I missing something?
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 25•8 years ago
|
||
If it's also used in AsmJSCache.cpp, I think it's fine to land it as is :)
Assignee | ||
Comment 27•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/905e3a9f755778aacfbf3308ff00e9f2ee22ffb5 Bug 1153978 - Part 0: Separate nsIPlatformInfo from nsIXULAppInfo. r=jst https://hg.mozilla.org/integration/mozilla-inbound/rev/08ee8e76c7039e07c91f34477121787c845c2cca Bug 1153978 - Part 1: Separate buildIdOp from AsmJSCacheOps. r=jandem,bz https://hg.mozilla.org/integration/mozilla-inbound/rev/8db1ac84d30e663f9487a9a54844dd044ebd934b Bug 1153978 - Part 2: Use the build id instead of the XDR_BYTECODE_VERSION constant. r=jandem
Assignee | ||
Comment 28•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1aebfd7287fd54b036825143c6c6ce257df94e Backed out changeset 8db1ac84d30e (bug 1153978) https://hg.mozilla.org/integration/mozilla-inbound/rev/39463641013346389bee9b585f8f08e8f4770a3a Backed out changeset 08ee8e76c703 (bug 1153978) https://hg.mozilla.org/integration/mozilla-inbound/rev/f6e85049facf7e3e6883945d51be189f8b8056de Backed out changeset 905e3a9f7557 (bug 1153978) for xpcshell test failure. CLOSED TREE
Assignee | ||
Comment 29•8 years ago
|
||
Still investigating the reason of the xpcshell crash tho, the mock app-info [1] implemented in JS seems to be the reason, that is registered as "@mozilla.org/xre/app-info;1" but doesn't implement Ci.nsIPlatformInfo. So, I need to add Ci.nsIPlatformInfo to its QueryInterface. The mock app-info code is copied around the tree, with several variants, and even in comm-central too. We should merge all of them into single implementation. will file a dedicated bug for merging. [1] https://dxr.mozilla.org/mozilla-central/rev/d1d47ba19ce9d46222030d491f9fe28dbf80be12/js/xpconnect/tests/unit/test_xpcomutils.js#126
Assignee | ||
Comment 30•8 years ago
|
||
Then, another issue is that somewhere in the code assumes that XDR VersionCheck doesn't fail for certain input, but now it can fail, as GetBuildId is fallible. that seems to be the reason of the crash, with assertion failure for pending exception.
Assignee | ||
Comment 31•8 years ago
|
||
ReadCachedScript calls JS_DecodeScript, and if it fails, ReadCachedScript returns NS_ERROR_OUT_OF_MEMORY. https://dxr.mozilla.org/mozilla-central/rev/d1d47ba19ce9d46222030d491f9fe28dbf80be12/js/xpconnect/loader/mozJSLoaderUtils.cpp#33 > scriptp.set(JS_DecodeScript(cx, buf.get(), len)); > if (!scriptp) > return NS_ERROR_OUT_OF_MEMORY; > return NS_OK; mozJSComponentLoader::ObjectForLocation calls ReadCachedScript, and just *ignores* the failure. So, in that case, the JS exception thrown by JS_DecodeScript is still pending, and the pending exception is caught by the assertion in BytecodeCompiler::compileScript. https://dxr.mozilla.org/mozilla-central/rev/d1d47ba19ce9d46222030d491f9fe28dbf80be12/js/xpconnect/loader/mozJSComponentLoader.cpp#691 > if (!mReuseLoaderGlobal) { > rv = ReadCachedScript(cache, cachePath, cx, mSystemPrincipal, &script); > } else { > rv = ReadCachedFunction(cache, cachePath, cx, mSystemPrincipal, > function.address()); > } > > if (NS_SUCCEEDED(rv)) { > LOG(("Successfully loaded %s from startupcache\n", nativePath.get())); > } else { > // This is ok, it just means the script is not yet in the > // cache. Could mean that the cache was corrupted and got removed, > // but either way we're going to write this out. > writeToCache = true; > } So, if it ignores the error, it should call JS_ClearPendingException.
Comment 32•8 years ago
|
||
> implemented in JS seems to be the reason, that is registered as "@mozilla.org/xre/app-info;1" Uh.... That contract is used from non-main threads in general (e.g. ParentRunnable::OpenCacheFileForWrite will run on a non-main thread). How is implementing it in JS in any way OK, unless we make sure to NEVER do anything with asmjs (or anything else that might use that contract from off the main thread) while using that mock? :( > mozJSComponentLoader::ObjectForLocation calls ReadCachedScript, and just *ignores* the failure. Yeah, we should probably jsapi.ReportException() in ObjectForLocation in the "cache and read failed" case.
Assignee | ||
Comment 33•8 years ago
|
||
What's the convention for handling a JS exception that is thrown by JSAPI, in XPCOM/XPConnect? If a function returns NS_* failure, can it imply a pending JS exception? (is there any caller that handles the exception?) or, the function should (report and) clear the exception before returning NS_* failure? if latter, I guess ReadCachedScript should handle the exception (by jsapi.ReportException() or JS_ClearPendingException), as currently WriteCachedScript calls JS_ClearPendingException. https://dxr.mozilla.org/mozilla-central/rev/d1d47ba19ce9d46222030d491f9fe28dbf80be12/js/xpconnect/loader/mozJSLoaderUtils.cpp#70 > nsresult > WriteCachedScript(StartupCache* cache, nsACString& uri, JSContext* cx, > nsIPrincipal* systemPrincipal, HandleScript script) > { > MOZ_ASSERT(JS_GetScriptPrincipals(script) == nsJSPrincipals::get(systemPrincipal)); > > uint32_t size; > void* data = JS_EncodeScript(cx, script, &size); > if (!data) { > // JS_EncodeScript may have set a pending exception. > JS_ClearPendingException(cx); > return NS_ERROR_FAILURE; > }
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 34•8 years ago
|
||
one more problematic place related to ReadCachedScript here. mozJSSubScriptLoader::DoLoadSubScriptWithOptions calls ReadCachedScript, and it also ignores the failure. https://dxr.mozilla.org/mozilla-central/rev/d1d47ba19ce9d46222030d491f9fe28dbf80be12/js/xpconnect/loader/mozJSSubScriptLoader.cpp#660 > nsresult > mozJSSubScriptLoader::DoLoadSubScriptWithOptions(const nsAString& url, > ... > if (cache && !options.ignoreCache) > rv = ReadCachedScript(cache, cachePath, cx, mSystemPrincipal, &script); > > // If we are doing an async load, trigger it and bail out. > if (!script && options.async) { > return ReadScriptAsync(uri, targetObj, options.charset, serv, > reusingGlobal, !!cache, retval); > } > > if (!script) { > rv = ReadScript(uri, cx, targetObj, options.charset, > static_cast<const char*>(uriStr.get()), serv, > principal, reusingGlobal, &script, &function); > } else { > cache = nullptr; > } > > if (NS_FAILED(rv) || (!script && !function)) > return rv; > > return EvalScript(cx, targetObj, retval, uri, !!cache, script, function); > }
Comment 35•8 years ago
|
||
> If a function returns NS_* failure, can it imply a pending JS exception? Yes, if it takes a JSContext argument. > if latter, I guess ReadCachedScript should handle the exception (by jsapi.ReportException() or JS_ClearPendingException), as > currently WriteCachedScript calls JS_ClearPendingException. In practice this only matters in terms of what gets shown in the browser console. If we think the error here will be actionable, we should report the exception. If not, we should just suppress it via jsapi.ClearException() on the caller, or JS_ClearPendingException in the callee if we want to preserve the old callee behavior of never leaving exceptions on the JSContext.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 36•8 years ago
|
||
thanks :) so, when a function ignores the failure of the function that takes JSContext, it should handle the exception, right? then, in this bug's case, XDR related JSAPI function can fail with several reasons (version mismatch, OOM, cannot write to buffer, etc), and previously all of them could be just ignored, but now VersionCheck can call JS function (in createInstance for app-info, and maybe platformBuildID getter?). in term of error reporting to browser console, version mismatch and some other pre-existing errors are not supposed to be reported there. now, it might be better reporting the error related to mock app-info, but it might make things complicated (especially, when to report, or how to distinguish them). I guess, the mock app-info is the special case, used only in xpcshell-test, and we could add some restriction to it, so, for now, I'm going to just clear the exception without reporting, to see if it works or not.
Comment 37•8 years ago
|
||
> so, when a function ignores the failure of the function that takes JSContext, > it should handle the exception, right? Probably yes. There is no hard and fast rule here... In practice, we should document what these functions do. > I guess, the mock app-info is the special case, used only in xpcshell-test, and we could add some > restriction to it, so, for now, I'm going to just clear the exception without reporting, to see > if it works or not. That makes sense to me. I still worry about this mock app-info causing problems if anyone does workers with wasm in xpcshell...
Assignee | ||
Comment 38•8 years ago
|
||
now it passes the try, with bug 1256088's patch, and extra 2 patches for this bug. https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe8c505b6e9c
Assignee | ||
Comment 39•8 years ago
|
||
Part 0 adds nsIPlatformInfo, that should be implemented in mock app-info, and now all mock app-info is merged to AppInfo.jsm in bug 1256088, so changed mock app-info in AppInfo.jsm to implement nsIPlatformInfo. also, plarformBuildID property is used in XDR encode/decode, so it should be the same value as original one. platformVersion can be modified to fit the testcase. Will fold this to Part 0.
Attachment #8731472 -
Flags: review?(gps)
Assignee | ||
Comment 40•8 years ago
|
||
Added JS_ClearPendingException when ignoring ReadCachedScript and ReadCachedFunction failure
Attachment #8731474 -
Flags: review?(bzbarsky)
Comment 41•8 years ago
|
||
Comment on attachment 8731474 [details] [diff] [review] Part 2.1: Clear pending exception thrown from XDR related APIs when ignoring ReadCachedScript failure. r=me
Attachment #8731474 -
Flags: review?(bzbarsky) → review+
Updated•8 years ago
|
Attachment #8731472 -
Flags: review?(gps) → review+
Assignee | ||
Comment 42•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/89d44b4257f1896af77af5940f6cb4787f7b8252 Bug 1153978 - Part 0: Separate nsIPlatformInfo from nsIXULAppInfo. r=jst,gps https://hg.mozilla.org/integration/mozilla-inbound/rev/f22eed4b7a39beeff05a30670e8339c3e29c8146 Bug 1153978 - Part 1: Separate buildIdOp from AsmJSCacheOps. r=jandem,bz https://hg.mozilla.org/integration/mozilla-inbound/rev/e20421e07ab1b041efaa6ca7ae51df8115e1fcd2 Bug 1153978 - Part 2: Clear pending exception thrown from XDR related APIs when ignoring ReadCachedScript failure. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/288b2aef2a412317e7092e333f8fa3fbdca49e37 Bug 1153978 - Part 3: Use the build id instead of the XDR_BYTECODE_VERSION constant. r=jandem
Comment 43•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/89d44b4257f1 https://hg.mozilla.org/mozilla-central/rev/f22eed4b7a39 https://hg.mozilla.org/mozilla-central/rev/e20421e07ab1 https://hg.mozilla.org/mozilla-central/rev/288b2aef2a41
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•