1329845 - Report missing properties on the self-hosting global during runtime initialization

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

Right now, we report failed JSOP_GETINTRINSIC operations at runtime. That means typos in self-hosted code can be quite tedious to debug.

It'd be much better to report them right after the self-hosted script has run, at which point the set of properties on the self-hosting global is fixed. Patch coming up.

Comment 1

[Till Schneidereit [:till]]

Attachment: Remove dead code from Intl.js

This is needed so we can land the next patch.

Comment 2

[Till Schneidereit [:till]]

Attachment: Report missing properties on the self-hosting global during runtime initialization

This does the actual reporting. I removed the runtime reporting since it can't hit anymore.

Comment 3

[André Bargull [:anba]]

(In reply to Till Schneidereit [till] from comment #1)
> Created attachment 8825238 [details] [diff] [review]
> Remove dead code from Intl.js
> 
> This is needed so we can land the next patch.

This is dead code because we don't (yet) implement the basic format matcher (bug 852837).

Comment 4

[Till Schneidereit [:till]]

(In reply to André Bargull from comment #3)
> This is dead code because we don't (yet) implement the basic format matcher
> (bug 852837).

I see, thanks for the explanation. I'll note that it has been dead code for close to 4 years by now, so perhaps we should still rip it out and add a note to bug 852837 about where to find it.

Comment 5

[André Bargull [:anba]]

(In reply to Till Schneidereit [till] from comment #4)
> (In reply to André Bargull from comment #3)
> > This is dead code because we don't (yet) implement the basic format matcher
> > (bug 852837).
> 
> I see, thanks for the explanation. I'll note that it has been dead code for
> close to 4 years by now, so perhaps we should still rip it out and add a
> note to bug 852837 about where to find it.

Yes, sounds good to me.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 8825240 [details] [diff] [review]
Report missing properties on the self-hosting global during runtime initialization

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

LGTM, just want clarification on the condition.

::: js/src/vm/SelfHosting.cpp
@@ +2799,5 @@
> +                // permanent, any attempt to look up a non-permanent atom will
> +                // fail. We should only see such atoms when code is looking
> +                // for properties on the self hosted global which aren't
> +                // present.
> +                if (name->isPermanentAtom() || !shg->lookupPure(id)) {

Is the condition wrong? The comment says looking up *non*-permanent atoms will fail, but the condition considers a name missing if it *is* permanent?

@@ -2879,5 @@
> -        MOZ_ASSERT(selfHostedObject->is<GlobalObject>());
> -        RootedValue value(cx, IdToValue(id));
> -        return ReportValueErrorFlags(cx, JSREPORT_ERROR, JSMSG_NO_SUCH_SELF_HOSTED_PROP,
> -                                     JSDVG_IGNORE_STACK, value, nullptr, nullptr, nullptr);
> -    }

Also add an assert that id isPermanentAtom() please.

Comment 7

[Till Schneidereit [:till]]

Attachment: Report missing properties on the self-hosting global during runtime initialization, v2

(In reply to Shu-yu Guo [:shu] from comment #6)
> Comment on attachment 8825240 [details] [diff] [review]
> Report missing properties on the self-hosting global during runtime
> initialization
> 
> Review of attachment 8825240 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM, just want clarification on the condition.
> 
> ::: js/src/vm/SelfHosting.cpp
> @@ +2799,5 @@
> > +                // permanent, any attempt to look up a non-permanent atom will
> > +                // fail. We should only see such atoms when code is looking
> > +                // for properties on the self hosted global which aren't
> > +                // present.
> > +                if (name->isPermanentAtom() || !shg->lookupPure(id)) {
> 
> Is the condition wrong? The comment says looking up *non*-permanent atoms
> will fail, but the condition considers a name missing if it *is* permanent?

Err, yes :( Inverting the condition causes us to report existing names as missing though. I don't understand why that is, perhaps they get marked as permanent after initialization has finished?

In any case, I just removed the permanentAtom stuff here, the lookup is what's important, after all.

> Also add an assert that id isPermanentAtom() please.

Done, with an explanatory comment.

Comment 8

[Shu-yu Guo [:shu]]

Comment on attachment 8826169 [details] [diff] [review]
Report missing properties on the self-hosting global during runtime initialization, v2

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

::: js/src/vm/SelfHosting.cpp
@@ +2918,5 @@
>  
> +    // Since all atoms used by self-hosting are marked as permanent, the only
> +    // reason we'd see a non-permanent atom here is code looking for
> +    // properties on the self hosted global which aren't present.
> +    // Since we ensure that that can't happen during startup, encountering

They aren't currently ensured as permanent in VerifyGlobalNames though. My previous comment about making this an assertion depended on the correct condition in VerifyGlobalNames, but since the isPermanentAtom thing can't be checked there for some reason, revert this back to the check?

Or, if this assertion doesn't fail, then... I just don't know what the isPermanentAtom thing is supposed to do.

In any case, like you said, the isPermanentAtom check isn't the important one. I'm fine with whatever doesn't crash.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/057c8ef1298a
Part 1: Remove dead code referencing invalid property from Intl.js. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/b45ad9e4e5d9
Part 2: Report missing properties on the self-hosting global during runtime initialization. r=shu

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/057c8ef1298a
https://hg.mozilla.org/mozilla-central/rev/b45ad9e4e5d9