This is subtle. > That is certainly the case, but we're in a region where 't' is known to be null and this was already the case before this patch, so I don't know a) why the analysis thinks this is a hazard It doesn't take the null-ness into account. What it sees is: if (MOZ_UNLIKELY(!t)) { if (allowGC && cx->runtime()->gc.lastDitchGC(cx)) { t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } if (!t) { ... } } MOZ_UNLIKELY(!t) just establishes that t is live at that point. Then it does a lastDitchGC, invalidating t -- which wouldn't be a problem if the old t were never used, but it is in the if (!t). (Note that there is only a hazard path when allowGC is true and lastDitchGC *fails*. If it succeeded, then t would be overwritten and the fact that it got invalidated first wouldn't matter.) and b) if it does consider this to be a hazard why it didn't already report a hazard for this method. Previously, the code was if (MOZ_UNLIKELY(!t && allowGC)) { if (!cx->helperThread()) { lastDitchGC(); // well, the equivalent t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } if (!t) { ...report OOM... } Here, lastDitchGC() again invalidates t, but its invalid value is unconditionally overwritten with tryNewTenuredThing() so there's no problem. > Steve, any idea how to proceed here? The analysis fix would be to allow "invalidated" pointers to be checked for null. (Hm... I wonder how far that could be loosened up? Clearly dereferencing a pointer is bad, and passing them to a function is bad. Comparing the pointer value is ok. Ugh... but hashing the pointer value is bad, and that would be hard to distinguish. So never mind.) The code fix would be to prevent the old invalidated t from being used after lastDitchGC is called. Some ways to do it: if (MOZ_UNLIKELY(!t)) { if (allowGC && cx->runtime()->gc.lastDitchGC(cx)) { t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } else { t = nullptr; } if (!t) { ...report OOM... } } or if (MOZ_UNLIKELY(!t)) { if (!allowGC) { return nullptr; } if (!cx->runtime()->gc.lastDitchGC(cx)) { ...report OOM... return nullptr; } t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); if (!t) { ...report OOM... } } or whatever.
Bug 1505622 Comment 5 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
This is subtle. > That is certainly the case, but we're in a region where 't' is known to be null and this was already the case before this patch, so I don't know a) why the analysis thinks this is a hazard It doesn't take the null-ness into account. What it sees is: if (MOZ_UNLIKELY(!t)) { if (allowGC && cx->runtime()->gc.lastDitchGC(cx)) { t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } if (!t) { ... } } MOZ_UNLIKELY(!t) just establishes that t is live at that point. Then it does a lastDitchGC, invalidating t -- which wouldn't be a problem if the old t were never used, but it is in the if (!t). (Note that there is only a hazard path when allowGC is true and lastDitchGC *fails*. If it succeeded, then t would be overwritten and the fact that it got invalidated first wouldn't matter.) and b) if it does consider this to be a hazard why it didn't already report a hazard for this method. Previously, the code was if (MOZ_UNLIKELY(!t && allowGC)) { if (!cx->helperThread()) { lastDitchGC(); // well, the equivalent t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } if (!t) { ...report OOM... } Here, lastDitchGC() again invalidates t, but its invalid value is unconditionally overwritten with tryNewTenuredThing() so there's no problem. > Steve, any idea how to proceed here? The analysis fix would be to allow "invalidated" pointers to be checked for null. (Hm... I wonder how far that could be loosened up? Clearly dereferencing a pointer is bad, and passing them to a function is bad. Comparing the pointer value is ok. Ugh... but hashing the pointer value is bad, and that would be hard to distinguish. So never mind.) The code fix would be to prevent the old invalidated t from being used after lastDitchGC is called. Some ways to do it: if (MOZ_UNLIKELY(!t)) { if (allowGC && cx->runtime()->gc.lastDitchGC(cx)) { t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } else { t = nullptr; } if (!t) { ...report OOM... } } or if (MOZ_UNLIKELY(!t)) { if (!allowGC) { return nullptr; } if (!cx->runtime()->gc.lastDitchGC(cx)) { ...report OOM... return nullptr; } t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); if (!t) { ...report OOM... } } or whatever.
This is subtle. > That is certainly the case, but we're in a region where 't' is known to be null and this was already the case before this patch, so I don't know a) why the analysis thinks this is a hazard It doesn't take the null-ness into account. What it sees is: if (MOZ_UNLIKELY(!t)) { if (allowGC && cx->runtime()->gc.lastDitchGC(cx)) { t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } if (!t) { ... } } MOZ_UNLIKELY(!t) just establishes that t is live at that point. Then it does a lastDitchGC, invalidating t -- which wouldn't be a problem if the old t were never used, but it is in the if (!t). (Note that there is only a hazard path when allowGC is true and lastDitchGC *fails*. If it succeeded, then t would be overwritten and the fact that it got invalidated first wouldn't matter.) > and b) if it does consider this to be a hazard why it didn't already report a hazard for this method. Previously, the code was if (MOZ_UNLIKELY(!t && allowGC)) { if (!cx->helperThread()) { lastDitchGC(); // well, the equivalent t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } if (!t) { ...report OOM... } Here, lastDitchGC() again invalidates t, but its invalid value is unconditionally overwritten with tryNewTenuredThing() so there's no problem. > Steve, any idea how to proceed here? The analysis fix would be to allow "invalidated" pointers to be checked for null. (Hm... I wonder how far that could be loosened up? Clearly dereferencing a pointer is bad, and passing them to a function is bad. Comparing the pointer value is ok. Ugh... but hashing the pointer value is bad, and that would be hard to distinguish. So never mind.) The code fix would be to prevent the old invalidated t from being used after lastDitchGC is called. Some ways to do it: if (MOZ_UNLIKELY(!t)) { if (allowGC && cx->runtime()->gc.lastDitchGC(cx)) { t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } else { t = nullptr; } if (!t) { ...report OOM... } } or if (MOZ_UNLIKELY(!t)) { if (!allowGC) { return nullptr; } if (!cx->runtime()->gc.lastDitchGC(cx)) { ...report OOM... return nullptr; } t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); if (!t) { ...report OOM... } } or whatever.
This is subtle. > That is certainly the case, but we're in a region where 't' is known to be null and this was already the case before this patch, so I don't know a) why the analysis thinks this is a hazard It doesn't take the null-ness into account. What it sees is: if (MOZ_UNLIKELY(!t)) { if (allowGC && cx->runtime()->gc.lastDitchGC(cx)) { t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } if (!t) { ... } } MOZ_UNLIKELY(!t) just establishes that t is live at that point. Then it does a lastDitchGC, invalidating t -- which wouldn't be a problem if the old t were never used, but it is in the if (!t). (Note that there is only a hazard path when allowGC is true and lastDitchGC *fails*. If it succeeded, then t would be overwritten and the fact that it got invalidated first wouldn't matter.) > and b) if it does consider this to be a hazard why it didn't already report a hazard for this method. Previously, the code was if (MOZ_UNLIKELY(!t && allowGC)) { if (!cx->helperThread()) { lastDitchGC(); // well, the equivalent t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } if (!t) { ...report OOM... } Here, lastDitchGC() again invalidates t, but its invalid value is unconditionally overwritten with tryNewTenuredThing() so there's no problem. > Steve, any idea how to proceed here? The analysis fix would be to allow "invalidated" pointers to be checked for null. (Hm... I wonder how far that could be loosened up? Clearly dereferencing a pointer is bad, and passing them to a function is bad. Comparing the pointer value is ok. Ugh... but hashing the pointer value is bad, and that would be hard to distinguish. So never mind.) The code fix would be to prevent the old invalidated t from being used after lastDitchGC is called. Some ways to do it: if (MOZ_UNLIKELY(!t)) { if (allowGC && cx->runtime()->gc.lastDitchGC(cx)) { t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } else { t = nullptr; } if (!t) { ...report OOM... } } or closer to the original: if (MOZ_UNLIKELY(!t) && allowGC) { if (cx->runtime()->gc.lastDitchGC(cx)) { t = tryNewTenuredThing<T, NoGC>(cx, kind, thingSize); } else { t = nullptr; } if (!t) { ...report OOM... } } or whatever. I guess the root of the issue here is the new boolean return value of lastDitchGC.