Stop doing string compares in SavedFrame::isSelfHosted to determine whether we're selfhosted

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

We have an atom, so should probably just do an atom compare.
Comment on attachment 8763694 [details] [diff] [review]
Do an atom equality compare instead of a string compare on the script filename string in SavedFrame::isSelfHosted

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

Thanks!

Since the JS::ubi::StackFrame interface changed, you will need to update the other implementation as well: https://dxr.mozilla.org/mozilla-central/rev/2ab57664bf7cafdd64e136e341527c275fc8c3aa/devtools/shared/heapsnapshot/DeserializedNode.h#299

::: js/src/vm/CommonPropertyNames.h
@@ +322,5 @@
>      /* Function names for properties named by symbols. */ \
>      macro(Symbol_iterator_fun, Symbol_iterator_fun, "[Symbol.iterator]") \
> +    /* Not really a property name, but we use this to compare to atomized
> +       script source strings */ \
> +    macro(self_hosted, self_hosted, "self-hosted") \

s/self_hosted/selfHosted/

We turn "foo-bar" into "fooBar" for other property names[0], so we should be consistent here.

[0] https://dxr.mozilla.org/mozilla-central/rev/2ab57664bf7cafdd64e136e341527c275fc8c3aa/js/src/vm/CommonPropertyNames.h#120
Attachment #8763694 - Flags: review?(nfitzgerald) → review+
> you will need to update the other implementation as well

Huh.  Could have sworn I'd done a full-tree compile with this, but clearly not!

> s/self_hosted/selfHosted/

Done.
But ok, now I see that HeapSnapshot.cpp has no cx in sight and wants to call isSelfHosted().. is that correct?  :(
Flags: needinfo?(nfitzgerald)
`frame.isSelfHosted()`
  `-> is called by `getProtobufStackFrame`
    `-> is called by `StreamWriter::writeNode` as part of the `CoreDumpWriter` interface
      `-> is called by `HeapSnapshotTraversalHandler::operator()`
        `-> `HeapSnapshotTraversalHandler` is initialized here[0] and has access to a cx at that time

So, that's a lot of threading cx through functions and classes to add, but is mostly furious typing without thinking.

[0] https://dxr.mozilla.org/mozilla-central/rev/23dc78b7b57e9f91798ea44c242a04e112c37db0/devtools/shared/heapsnapshot/HeapSnapshot.cpp#1395

One potential complication I thought of that turns out to be ok: we do NOT need to recanonicalize a deserialized heap snapshot's stack frame's source to the "self-hosted" atom if the stack frame's source is "self-hosted" because we actually serialize and deserialize the isSelfHosted data separately and we use that for JS::ubi::Concrete<DeserializedStackFrame>::isSelfHosted[1].

[1] https://dxr.mozilla.org/mozilla-central/rev/23dc78b7b57e9f91798ea44c242a04e112c37db0/devtools/shared/heapsnapshot/DeserializedNode.h#299
Flags: needinfo?(nfitzgerald)
Oh, I'm an idiot.  StreamWriter has a JSContext* _member_ that I can use.  I'll just do that.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e8cf178dae8
Do an atom equality compare instead of a string compare on the script filename string in SavedFrame::isSelfHosted.  r=fitzgen
https://hg.mozilla.org/mozilla-central/rev/6e8cf178dae8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.