Closed Bug 1329845 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected
firefox54 --- fixed

People

(Reporter: till, Assigned: till)

Details

Attachments

(2 files, 1 obsolete file)

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.
This is needed so we can land the next patch.
Attachment #8825238 - Flags: review?(shu)
This does the actual reporting. I removed the runtime reporting since it can't hit anymore.
Attachment #8825240 - Flags: review?(shu)
(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).
(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.
(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.
Attachment #8825238 - Flags: review?(shu) → review+
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.
Attachment #8825240 - Flags: review?(shu)
(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.
Attachment #8825240 - Attachment is obsolete: true
Attachment #8826169 - Flags: review?(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.
Attachment #8826169 - Flags: review?(shu) → review+
Pushed by tschneidereit@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/057c8ef1298a
https://hg.mozilla.org/mozilla-central/rev/b45ad9e4e5d9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.