1201620 - SavedFrame stacks should be structured cloneable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

It will make our lives a lot easier when running census analyses broken down by allocation stack in a worker thread.

Comment 1

[Morgan Phillips [:mrrrgn]]

All your bug are belong to me.

Comment 2

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

Hey Morgan, how's it coming? Any questions I can help answer, or anything like that?

Comment 3

[Morgan Phillips [:mrrrgn]]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #2)
> Hey Morgan, how's it coming? Any questions I can help answer, or anything
> like that?

Sorry, I've gotten bogged down in RelEng stuff and not made good progress here. I'll let you know if I don't soon, or, if I get stuck.

If this is blocking you please yell at me and re-assign.

Comment 4

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

(In reply to Morgan Phillips [:mrrrgn] from comment #3)
> (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #2)
> > Hey Morgan, how's it coming? Any questions I can help answer, or anything
> > like that?
> 
> Sorry, I've gotten bogged down in RelEng stuff and not made good progress
> here. I'll let you know if I don't soon, or, if I get stuck.
> 
> If this is blocking you please yell at me and re-assign.

This is the last platform bug blocking v1 of the new memory panel, so I think I have to steal it back, sorry.

Ping me on irc sometime and I can find some other good bugs for you.

Comment 5

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

Attachment: Make SavedFrame stacks structured cloneable

Comment 6

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

Just feedback for now because I'm still huntint down a SIGABRT after the new test passes while destroying the context and shutting down the test...

testStructuredClone_SavedFrame
TEST-PASS | testStructuredClone_SavedFrame | ok

Program received signal SIGABRT, Aborted.
0x00007fff971c3286 in __pthread_kill ()
(gdb) bt
#0  0x00007fff971c3286 in __pthread_kill ()
#1  0x00007fff9678742f in pthread_kill ()
#2  0x00007fff94b02bf3 in __abort ()
#3  0x00007fff94b034d1 in __stack_chk_fail ()
#4  0x0000000100b7ea26 in js::DestroyContext (cx=0x103531400, mode=js::DCM_FORCE_GC) at jscntxt.cpp:189
#5  0x0000000100b7e79b in JS_DestroyContext (cx=0x103531400) at jsapi.cpp:801
#6  0x00000001000b780e in JSAPITest::uninit (this=0x101dbac40) at tests.cpp:44
#7  0x00000001000b806c in main (argc=1, argv=0x7fff5fbff538) at tests.cpp:134

Comment 7

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

Ok, so I finally figured it out thanks to some help from jimb (thank you so much!!!).

The test case uses a stack allocated mock principals subclass and then we would create SavedFrame objects using that principals, their lifetime extended past the frame the principals were stack allocated in, we would do a gc when destorying the cx in JSAPITest::uninit, this would run the SavedFrame finalizers which drop the principals they hold on to. The principals pointer is no longer valid because we popped the frame, and it just happens to be the location where the rip was being pushed, so when we left js::DestroyContext we would jump to a bogus location.

Phew!

Now that I know what the issue is, the fix is actually really easy, though!

Comment 8

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

Attachment: Make SavedFrame stacks structured cloneable

Comment 9

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

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

Comment 10

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

Attachment: Make SavedFrame stacks structured cloneable

Comment 11

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

That should fix the rooting hazard that was in the test. Not sure what those other failures are, the logs really aren't helping at all. Here's to hoping that they're just because of the rooting hazard.

Here is a new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8ac2b820275

Comment 12

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

Attachment: Make SavedFrame stacks structured cloneable

Comment 13

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

This is just a fix for a bad rebase.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b85d2de0f76

Comment 14

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8670983 [details] [diff] [review]
Make SavedFrame stacks structured cloneable

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

I really wish I had landed the patch for cleaning up the special object handling. It allowed you to register handlers for things like Set, Map, SavedFrame, etc. instead of embedding them in the code.

Oh well.

::: js/src/vm/StructuredClone.cpp
@@ +2094,5 @@
> +            if (!startRead(&parent) ||
> +                !(parent.isObject() ? parent.toObject().is<SavedFrame>() : parent.isNull()))
> +            {
> +                return false;
> +            }

This seems a little dense to me. Can you expand it out to something like

  if (!startRead(&parent))
      return false;

  SavedFrame* parentFrame;
  if (parent.isNull()) {
      parentFrame = nullptr;
  } else if (parent.isObject() && parent.toObject().is<SavedFrame>()) {
      parentFrame = &parent.toObject().as<SavedFrame>();
  } else {
      return false;
  }
  ...initParent(parentFrame);

Comment 15

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

Attachment: Make SavedFrame stacks structured cloneable

Comment 16

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

Comment on attachment 8671093 [details] [diff] [review]
Make SavedFrame stacks structured cloneable

Jan, can you review the indexeddb related changes? I bumped the structured cloning version number, which triggered cascading version bumps in indexeddb. However, AFAICT, there shouldn't be any migrations needed because the structured clone changes are strictly additions to the SCTAG set and the set of structured cloneable objects. Thanks!

Comment 17

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

Attachment: Make SavedFrame stacks structured cloneable

Comment 18

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

sfink: This version fixes the issues uncovered in bug 1201621 that we talked about on irc.

Jan: everything from comment 16 still applies.

Comment 19

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

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

Comment 20

[Jan Varga [:janv]]

Comment on attachment 8671606 [details] [diff] [review]
Make SavedFrame stacks structured cloneable

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

Hm, don't you need to add UpgradeSchemaFrom21_0To22_0() ?
Look at UpgradeSchemaFrom15_0To16_0()

Comment 21

[Jan Varga [:janv]]

And the white space fixes shouldn't be part of the patch I think.

Comment 22

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

Attachment: Make SavedFrame stacks structured cloneable

Comment 23

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

Changes from last revision:

* UpgradeSchemaFrom21_0To22_0
* Re-add trailing white space

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

Comment 24

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8671640 [details] [diff] [review]
Make SavedFrame stacks structured cloneable

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

Yes, much better.

Comment 25

[Jan Varga [:janv]]

Comment on attachment 8671640 [details] [diff] [review]
Make SavedFrame stacks structured cloneable

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

r=me on dom/indexedDB changes

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/530b538c480d

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e802ab9dd60c

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/530b538c480d
https://hg.mozilla.org/mozilla-central/rev/e802ab9dd60c