Closed Bug 1248948 Opened 4 years ago Closed 4 years ago

crash in js::SavedStacks::adoptAsyncStack

Categories

(Core :: JavaScript Engine, defect, critical)

45 Branch
Unspecified
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox44 --- unaffected
firefox45 + wontfix
firefox46 + wontfix
firefox47 + fixed
firefox48 --- fixed

People

(Reporter: robin, Assigned: fitzgen)

References

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(2 files, 3 obsolete files)

Hit this reliably by visiting https://www.augur.io/ , opening dev tools, showing the network tab and forcing a reload (on current Nightly).

Crash log: https://crash-stats.mozilla.com/report/index/c0bda5f9-c505-40ae-9f44-bbc862160217
Reg range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=a271e4ef0c51dc90cf11adf65bcbb28bcdc776f5&tochange=096b5587a7fa16e7bd1ef5c025cd8e3f13fcf897

Victor Porof — Bug 1218078 - Show onload and DOMContentLoaded markers in the netmonitor's frontend, r=smaug, jsantell, tromey
Blocks: 1218078
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(vporof)
Version: Trunk → 45 Branch
Tracking for 45+, reproducible crash, recent regression.
That commit doesn't have any C++ changes. Most likely it exercises some code paths that were already problematic. I'm not really sure who to ping instead for SavedStacks::adoptAsyncStack crashes. Maybe :shu?
Flags: needinfo?(vporof)
(In reply to Victor Porof [:vporof][:vp] from comment #3)
> That commit doesn't have any C++ changes.

...mean to say any *relevant* changes.
Naveed can you help find someone to look into this crash? I hate to keep tracking a bug that no one will take on.
Flags: needinfo?(nihsanullah)
Terrence the group_ member maybe bad in the compartment() call. They have steps to repro above. Can you please take a look?
Flags: needinfo?(nihsanullah) → needinfo?(terrence)
Forwarding ni? to someone who is more familiar with the saved stacks stuff.
Flags: needinfo?(terrence) → needinfo?(nfitzgerald)
Looking into this.
Assignee: nobody → nfitzgerald
Status: NEW → ASSIGNED
Bobby, we end up failing this assertion[0] because we still end up with a wrapper Proxy when unwrapping in isSavedFrameOrWrapperAndNotProto here[1]. If we change that CheckedUnwrap into an UncheckedUnwrap, then we get a SavedFrame object and everything seems to work. However, I don't fully understand the implications of changing a CheckedUnwrap to an UncheckedUnwrap. All the existing SavedFrame stacks code uses CheckedUnwrap. All SavedFrame objects and the SavedFrame.prototype are Object.freeze'd, so I'm not terribly worried about accidentally executing content scripts.

Can you cast some light on whether it is safe for the SavedFrame stacks code to use UncheckedUnwrap and whether or not all the other unwrapping calls should switch to the unchecked variant?

Thanks!

[0] https://dxr.mozilla.org/mozilla-central/source/js/src/vm/SavedStacks-inl.h?from=assertObjectIsSavedFrameOrWrapper#23
[1] https://dxr.mozilla.org/mozilla-central/source/js/src/vm/SavedFrame.h#128
Flags: needinfo?(nfitzgerald) → needinfo?(bobbyholley)
CheckedUnwrap returns null if the security policy vetoes the action, rather than returning a wrapper. What kind of proxy do you encounter? Is it an outer window proxy? UncheckedUnwrap _can_ unwrap those, but only if you pass stopAtWindowProxy=false as the default argument.

In general, you should always used CheckedUnwrap and not use UncheckedUnwrap unless you know exactly what you're doing.
Flags: needinfo?(bobbyholley)
Indeed, passing `stopAtWindowProxy = false` solved the issues!
Attachment #8728528 - Flags: review?(bobbyholley)
Comment on attachment 8728528 [details] [diff] [review]
Do not stop at window proxies when unwrapping SavedFrame wrappers

Review of attachment 8728528 [details] [diff] [review]:
-----------------------------------------------------------------

I don't see why you actually need to unwrap WindowProxies. This is basically because you have an assert against wrappers in isSavedFrameAndNotProto, right? Why not just fix that assert to assert !IsCrossCompartmentWrapper?
Attachment #8728528 - Flags: review?(bobbyholley) → review-
(In reply to Bobby Holley (busy) from comment #14)
> I don't see why you actually need to unwrap WindowProxies. This is basically
> because you have an assert against wrappers in isSavedFrameAndNotProto,
> right? Why not just fix that assert to assert !IsCrossCompartmentWrapper?

We aren't specifically asserting against wrappers, we are asserting that we have a SavedFrame object and the SavedFrame object is not the SavedFrame.prototype SavedFrame object. For example, if you passed in an ArrayObject then !IsCrossCompartmentWrapper would be true, but it still wouldn't be safe to proceed as the slots we are accessing may or may not exist and definitely won't contain the expected things.
Flags: needinfo?(bobbyholley)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #15)
> (In reply to Bobby Holley (busy) from comment #14)
> > I don't see why you actually need to unwrap WindowProxies. This is basically
> > because you have an assert against wrappers in isSavedFrameAndNotProto,
> > right? Why not just fix that assert to assert !IsCrossCompartmentWrapper?
> 
> We aren't specifically asserting against wrappers, we are asserting that we
> have a SavedFrame object and the SavedFrame object is not the
> SavedFrame.prototype SavedFrame object. For example, if you passed in an
> ArrayObject then !IsCrossCompartmentWrapper would be true, but it still
> wouldn't be safe to proceed as the slots we are accessing may or may not
> exist and definitely won't contain the expected things.

But unwrapping window proxies won't help you, because a window proxy is still not a SavedFrame object. So my point is that you're still going to need to do a type-check somewhere, and so you might as well fail the type check on the window proxy, rather than unwrapping it and failing on the underling window.
Flags: needinfo?(bobbyholley)
STR without this patch applied no longer lead to a crash for me, so maybe the patch is unrelated to the crash no longer happening.
I can reproduce this by going to https://diafygi.github.io/webrtc-ips/, opening the network tab in the devtools and reloading.
Nick can you please try Tom's STR?
Flags: needinfo?(nfitzgerald)
Unfortunately, I can't reproduce on that page. Going to try and pull down the latest m-c, since my loca copy is about a week old.
Flags: needinfo?(nfitzgerald)
Can't reproduce on the latest m-c either... :-/
(In reply to Tom Schuster [:evilpie] from comment #18)
> I can reproduce this by going to https://diafygi.github.io/webrtc-ips/,
> opening the network tab in the devtools and reloading.

It crashes for me:
https://crash-stats.mozilla.com/report/index/37bb3383-54f6-46b7-a639-5e1862160322
Adding more and release-mode asserts in bug 1258535, which is pretty similar.
See Also: → 1258535
Tom does the new set of asserts give additional info?
Flags: needinfo?(evilpies)
Oh, I think it does! Here is a new crash report: https://crash-stats.mozilla.com/report/index/996540a0-5f45-48c9-9e5c-3dc802160401.
Flags: needinfo?(evilpies)
Nick does this new crash report info help?
Flags: needinfo?(nfitzgerald)
It gives enough that I can make a work around, but I still don't understand the root cause.
Flags: needinfo?(nfitzgerald)
Attachment #8728528 - Attachment is obsolete: true
Comment on attachment 8737971 [details] [diff] [review]
Don't pass non-SavedFrame objects to SavedFrame JSAPI functions

I don't think this the right way to fix this. To me it looks like asyncStack is corrupt: https://pastebin.mozilla.org/8866425.

Btw, a bit above:
if (mAsyncStack.isObject() && !mAsyncStack.isNullOrUndefined() &&
    !mAsyncCause.IsEmpty()) {
JS::Rooted<JSObject*> asyncStack(aCx, mAsyncStack.toObjectOrNull());

mAsyncStack.isObject() && !mAsyncStack.isNullOrUndefined() can just be isObject(), isObject() always implies !isNullOrUndefined().
mAsyncStack.toObjectOrNull() should be toObject().

All those JS_ClearPendingException calls seem necessary, but aren't nice.
Attachment #8737971 - Flags: review?(evilpies)
Comment on attachment 8737971 [details] [diff] [review]
Don't pass non-SavedFrame objects to SavedFrame JSAPI functions

Review of attachment 8737971 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch.

It looks good to me.
I also don't understand how this can happen but the workaround seems reasonable.
Attachment #8737971 - Flags: review?(ttromey) → review+
Comment on attachment 8737971 [details] [diff] [review]
Don't pass non-SavedFrame objects to SavedFrame JSAPI functions

Review of attachment 8737971 [details] [diff] [review]:
-----------------------------------------------------------------

Seems like the patch actually fixes the crash for me. It's rejecting some kind of Proxy.

::: js/src/vm/SavedStacks.cpp
@@ +933,5 @@
> +JS_PUBLIC_API(bool)
> +IsSavedFrame(JSObject* obj)
> +{
> +    if (!obj)
> +        return true;

return false
Attachment #8737971 - Flags: review?(evilpies)
Attachment #8737971 - Flags: review?(evilpies) → review+
Attachment #8737971 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/1e5447cbadfa
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8738268 [details] [diff] [review]
Don't pass non-SavedFrame objects to SavedFrame JSAPI functions

Approval Request Comment
[Feature/regressing bug #]:
Bug 1218078, I guess?

[User impact if declined]:
Intermittent crash when using devtools network monitor.

[Describe test coverage new/current, TreeHerder]:
The network monitor has a test suite, but I am unfamiliar with the network monitory in general (and haven't been able to repro the crash either). It is unclear to me how much of these code paths within spidermonkey and gecko are exercised by the network monitor tests.

[Risks and why]:
Relatively little. Despite the iffy test situation, this patch is very simple and exists only to minimize and mitigate crashes.

[String/UUID change made/needed]:
None.
Attachment #8738268 - Flags: approval-mozilla-aurora?
Comment on attachment 8738268 [details] [diff] [review]
Don't pass non-SavedFrame objects to SavedFrame JSAPI functions

Crash fix, Aurora47+
Attachment #8738268 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hello Robin, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(robin)
Nick, is this something worth uplifting to Beta or is this a seldom-hit crash and therefore not worth taking a risk in the late Beta46 cycle?
Flags: needinfo?(nfitzgerald)
It is pretty low risk, but there are only 60 crashes with this signature altogether on crash-stacks. Not sure it is with it late in a beta cycle.
Flags: needinfo?(nfitzgerald)
I’m away from my computer for the next two weeks but I’lol verify after that. Alternatively if anyone else was originally able to reproduce and can’t now I’m happy to take their word for it.
Flags: needinfo?(robin)
This should fix the bustage. Was just missing the "js::" namespace prefix.
Flags: needinfo?(nfitzgerald)
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #41)
> It is pretty low risk, but there are only 60 crashes with this signature
> altogether on crash-stacks. Not sure it is with it late in a beta cycle.

Makes sense.
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #45)
> Created attachment 8740043 [details] [diff] [review]
> Bug-1248948---Dont-pass-non-SavedFrame-objects-to-.patch
> 
> This should fix the bustage. Was just missing the "js::" namespace prefix.

has problems to apply:

dding 1248948 to series file
renamed 1248948 -> Bug-1248948---Dont-pass-non-SavedFrame-objects-to-.patch
applying Bug-1248948---Dont-pass-non-SavedFrame-objects-to-.patch
patching file docshell/base/timeline/JavascriptTimelineMarker.h
Hunk #1 FAILED at 56
1 out of 1 hunks FAILED -- saving rejects to file docshell/base/timeline/JavascriptTimelineMarker.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh Bug-1248948---Dont-pass-non-SavedFrame-objects-to-.patch
Flags: needinfo?(nfitzgerald)
Attachment #8740043 - Attachment is obsolete: true
See newly rebased patch.
Flags: needinfo?(nfitzgerald)
You need to log in before you can comment on or make changes to this bug.