Bug 118878 – fourthTier: Structure should be able to tell you if it's valid to load at a given offset from any object with that structure

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Thus, if we have a GetByOffset with a given offset, we can check if the object was proved to have a structure that has that offset; if it does, then the GetByOffset can proceed.

[Filip Pizlo]

Created attachment 207054 [details]
the patch

[Filip Pizlo]

Created attachment 207110 [details]
the patch

[Filip Pizlo]

Created attachment 207125 [details]
the patch

Just improved the changelog.

[Oliver Hunt]

Comment on attachment 207125 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207125&action=review

> Source/JavaScriptCore/ChangeLog:35
> +        that could transitively try to compute the hash from the source code. The source
> +        code is a string that may be lazily computed, and that involves all manner of thread
> +        unsafe things.

Do we have any assertions that would catch this?

[Filip Pizlo]

(In reply to comment #5)
> (From update of attachment 207125 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207125&action=review
> 
> > Source/JavaScriptCore/ChangeLog:35
> > +        that could transitively try to compute the hash from the source code. The source
> > +        code is a string that may be lazily computed, and that involves all manner of thread
> > +        unsafe things.
> 
> Do we have any assertions that would catch this?

Yes, that's how I caught it.  StringImpl's ref/deref assert that you're not in the compiler thread.

[Geoffrey Garen]

Comment on attachment 207125 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=207125&action=review

> Source/JavaScriptCore/runtime/Structure.h:228
>      bool isValidOffset(PropertyOffset offset) const
>      {
> -        return offset >= firstValidOffset()
> -            && offset <= lastValidOffset();
> +        return JSC::isValidOffset(offset)
> +            && (offset < m_inlineCapacity
> +                || (offset >= firstOutOfLineOffset && offset <= m_offset));
>      }

I think it's worth a comment here explaining that a "valid" offset isn't necessarily a valid JSValue -- it's just an offset that, if you load from it, you won't crash.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/152909

[Filip Pizlo]

(In reply to comment #7)
> (From update of attachment 207125 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=207125&action=review
> 
> > Source/JavaScriptCore/runtime/Structure.h:228
> >      bool isValidOffset(PropertyOffset offset) const
> >      {
> > -        return offset >= firstValidOffset()
> > -            && offset <= lastValidOffset();
> > +        return JSC::isValidOffset(offset)
> > +            && (offset < m_inlineCapacity
> > +                || (offset >= firstOutOfLineOffset && offset <= m_offset));
> >      }
> 
> I think it's worth a comment here explaining that a "valid" offset isn't necessarily a valid JSValue -- it's just an offset that, if you load from it, you won't crash.

Interesting.  While that's not what I intended for this method, its users are indeed file with the "you won't crash" part.  I'll change it though, in a separate bug.

[Filip Pizlo]

(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 207125 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=207125&action=review
> > 
> > > Source/JavaScriptCore/runtime/Structure.h:228
> > >      bool isValidOffset(PropertyOffset offset) const
> > >      {
> > > -        return offset >= firstValidOffset()
> > > -            && offset <= lastValidOffset();
> > > +        return JSC::isValidOffset(offset)
> > > +            && (offset < m_inlineCapacity
> > > +                || (offset >= firstOutOfLineOffset && offset <= m_offset));
> > >      }
> > 
> > I think it's worth a comment here explaining that a "valid" offset isn't necessarily a valid JSValue -- it's just an offset that, if you load from it, you won't crash.
> 
> Interesting.  While that's not what I intended for this method, its users are indeed file with the "you won't crash" part.  I'll change it though, in a separate bug.

https://bugs.webkit.org/show_bug.cgi?id=118911