1248948 - crash in js::SavedStacks::adoptAsyncStack

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robin Whittleton]

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

Comment 1

[Loic]

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

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 45+, reproducible crash, recent regression.

Comment 3

[Victor Porof [:vporof][:vp]]

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?

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to Victor Porof [:vporof][:vp] from comment #3)
> That commit doesn't have any C++ changes.

...mean to say any *relevant* changes.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Naveed can you help find someone to look into this crash? I hate to keep tracking a bug that no one will take on.

Comment 6

[Naveed Ihsanullah [:naveed]]

Terrence the group_ member maybe bad in the compartment() call. They have steps to repro above. Can you please take a look?

Comment 7

[Terrence Cole [:terrence]]

Forwarding ni? to someone who is more familiar with the saved stacks stuff.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Looking into this.

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 10

[Bobby Holley (:bholley)]

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.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Too late for 45.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Do not stop at window proxies when unwrapping SavedFrame wrappers

Indeed, passing `stopAtWindowProxy = false` solved the issues!

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=885706d28f9d

Comment 14

[Bobby Holley (:bholley)]

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?

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 16

[Bobby Holley (:bholley)]

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

Comment 17

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 18

[Tom S [:evilpie]]

I can reproduce this by going to https://diafygi.github.io/webrtc-ips/, opening the network tab in the devtools and reloading.

Comment 19

[Naveed Ihsanullah [:naveed]]

Nick can you please try Tom's STR?

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 21

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Can't reproduce on the latest m-c either... :-/

Comment 22

[Loic]

(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

Comment 23

[Tom S [:evilpie]]

Same stack for me: https://crash-stats.mozilla.com/report/index/a51b7eed-4a21-4dcb-9176-8a1612160319

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Adding more and release-mode asserts in bug 1258535, which is pretty similar.

Comment 25

[Naveed Ihsanullah [:naveed]]

Tom does the new set of asserts give additional info?

Comment 26

[Tom S [:evilpie]]

Oh, I think it does! Here is a new crash report: https://crash-stats.mozilla.com/report/index/996540a0-5f45-48c9-9e5c-3dc802160401.

Comment 27

[Naveed Ihsanullah [:naveed]]

Nick does this new crash report info help?

Comment 28

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

It gives enough that I can make a work around, but I still don't understand the root cause.

Comment 29

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Don't pass non-SavedFrame objects to SavedFrame JSAPI functions

Comment 30

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=00a4d56ddaab

Comment 31

[Tom S [:evilpie]]

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.

Comment 32

[Tom Tromey :tromey]

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.

Comment 33

[Tom S [:evilpie]]

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

Comment 34

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Don't pass non-SavedFrame objects to SavedFrame JSAPI functions

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e5447cbadfa

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1e5447cbadfa

Comment 37

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8738268 [details] [diff] [review]
Don't pass non-SavedFrame objects to SavedFrame JSAPI functions

Crash fix, Aurora47+

Comment 39

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hello Robin, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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?

Comment 41

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 42

[Robin Whittleton]

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.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5f050838a6a2

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca4e2efb2ef9

https://treeherder.mozilla.org/logviewer.html#?job_id=2318482&repo=mozilla-aurora

Comment 45

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Bug-1248948---Dont-pass-non-SavedFrame-objects-to-.patch

This should fix the bustage. Was just missing the "js::" namespace prefix.

Comment 46

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 47

[Carsten Book [:Tomcat]]

(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

Comment 48

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: rebased on latest aurora

Comment 49

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

See newly rebased patch.

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/359c46892e79