Bug 215208 – JS API access capture and their linking with executing scripts

• Status: NEW
• Component: New Bugs

[Umar Iqbal]

JS API access capture and their linking with executing scripts

[Umar Iqbal]

Created attachment 406070 [details]
Patch

[Umar Iqbal]

Created attachment 406084 [details]
Patch

[Umar Iqbal]

Created attachment 406118 [details]
Patch

[Kate Cheney]

Comment on attachment 406118 [details]
Patch

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

Nice job on the bots!

I see a lot of repetition in the if (RuntimeEnabledFeatures::sharedFeatures().webAPIStatisticsEnabled()) checks. Is it possible to split that code out into a separate function that takes a String argument?

I mostly left comments on how to combine some lines of code.

> Source/WebCore/loader/ExecutionStack.h:62
> +    // This HashMap maintains the mapping between FrameID, ScriptID, and Script URLs

Comment is missing a period.

> Source/WebCore/loader/ExecutionStack.h:65
> +    // This HashMap maintains the APIs called by a script in a docuemnt.

Oops, typo -- this should be document.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:104
> +    RELEASE_ASSERT(!isEphemeral());

Do we want this to crash in this case? It shouldn't be possible to hit this code, but maybe a debug assert would be better here?

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:281
> +    RegistrableDomain registrableDomain { document.url() };

I think you can delete this line and just initialize in the function call, see below.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:282
> +    uint64_t frameID { document.frameID()->toUInt64() };

You can use auto here, it's shorter.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:284
> +    auto& executionStack = ensureExecutionStackForRegistrableDomain(registrableDomain);

auto& executionStack = ensureExecutionStackForRegistrableDomain(RegistrableDomain{ document.url() });

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:286
> +    // register script id with script url for each frame

Comments usually start with a capital letter and end with a period.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:290
> +        // add will ensure that the exisitng script URL does not get overwritten.

Ditto here about capitalization.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:325
> +    uint64_t frameID { document.frameID()->toUInt64() };

Ditto about using auto.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:327
> +    auto& executionStack = ensureExecutionStackForRegistrableDomain(registrableDomain);

auto& executionStack = ensureExecutionStackForRegistrableDomain(RegistrableDomain{ document.url() });

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:334
> +            temp = executingScriptsStackIterator->value.last();

auto temp = executingScriptsStackIterator->value.last(); ?

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:359
> +    auto& executionStack = ensureExecutionStackForRegistrableDomain(registrableDomain);

Same comments as above.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:378
> +    auto& executionStack = ensureExecutionStackForRegistrableDomain(registrableDomain);

Same comments as above.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:419
> +    auto& statistics = ensureExecutionStackForRegistrableDomain(registrableDomain);

Same comments here as above -- you can use auto for the frameID and call RegistrableDomain { document.url() } in the function call.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:430
> +            documentWithScriptsIterator->value.set(activeScriptID, apis);

I think you can use WTFMove(apis) here and subsequent calls.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:437
> +        apisExecutingInScript.set(activeScriptID, apis);

Ditto, WTFMove(apis)

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:455
> +    auto& statistics = ensureExecutionStackForRegistrableDomain(registrableDomain);

auto& statistics = ensureExecutionStackForRegistrableDomain(RegistrableDomain { document.url() });

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:456
> +    String APIJSONString = statistics.toJSONString(frameID);

String APIJSONString = statistics.toJSONString(frameID { document.frameID()->toUInt64() });

[Umar Iqbal]

Created attachment 406198 [details]
Patch

[Umar Iqbal]

Created attachment 406201 [details]
Patch

[Keith Miller]

Comment on attachment 406201 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        These hooks capture eval/Function, callbacks, and microtasks execution.

Can you add a note that this isn't going to ship in production. Otherwise, this code looks very scary from a performance POV :P

[Umar Iqbal]

(In reply to Keith Miller from comment #7)
> Comment on attachment 406201 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406201&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        These hooks capture eval/Function, callbacks, and microtasks execution.
> 
> Can you add a note that this isn't going to ship in production. Otherwise,
> this code looks very scary from a performance POV :P

Thanks for the feedback. I will update the change log to reflect that.

[Umar Iqbal]

(In reply to katherine_cheney from comment #4)
> Comment on attachment 406118 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406118&action=review
> 
> Nice job on the bots!
> 
> I see a lot of repetition in the if
> (RuntimeEnabledFeatures::sharedFeatures().webAPIStatisticsEnabled()) checks.
> Is it possible to split that code out into a separate function that takes a
> String argument?

Moved the checks inside the if statement block to WebCore::WebResourceLoadObserver

> I mostly left comments on how to combine some lines of code.
> 
> > Source/WebCore/loader/ExecutionStack.h:62
> > +    // This HashMap maintains the mapping between FrameID, ScriptID, and Script URLs
> 
> Comment is missing a period.

Fixed.

> 
> > Source/WebCore/loader/ExecutionStack.h:65
> > +    // This HashMap maintains the APIs called by a script in a docuemnt.
> 
> Oops, typo -- this should be document.

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:104
> > +    RELEASE_ASSERT(!isEphemeral());
> 
> Do we want this to crash in this case? It shouldn't be possible to hit this
> code, but maybe a debug assert would be better here?
> 

I used RELEASE_ASSERT because the WebResourceLoadObserver ::ensureResourceStatisticsForRegistrableDomain also uses it. 

> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:281
> > +    RegistrableDomain registrableDomain { document.url() };
> 
> I think you can delete this line and just initialize in the function call,
> see below.

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:282
> > +    uint64_t frameID { document.frameID()->toUInt64() };
> 
> You can use auto here, it's shorter.
> 

Fixed.

> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:284
> > +    auto& executionStack = ensureExecutionStackForRegistrableDomain(registrableDomain);
> 
> auto& executionStack =
> ensureExecutionStackForRegistrableDomain(RegistrableDomain{ document.url()
> });

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:286
> > +    // register script id with script url for each frame
> 
> Comments usually start with a capital letter and end with a period.
> 

Fixed.

> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:290
> > +        // add will ensure that the exisitng script URL does not get overwritten.
> 
> Ditto here about capitalization.
> 

Fixed.

> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:325
> > +    uint64_t frameID { document.frameID()->toUInt64() };
> 
> Ditto about using auto.

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:327
> > +    auto& executionStack = ensureExecutionStackForRegistrableDomain(registrableDomain);
> 
> auto& executionStack =
> ensureExecutionStackForRegistrableDomain(RegistrableDomain{ document.url()
> });

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:334
> > +            temp = executingScriptsStackIterator->value.last();
> 
> auto temp = executingScriptsStackIterator->value.last(); ?
> 

Fixed.

> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:359
> > +    auto& executionStack = ensureExecutionStackForRegistrableDomain(registrableDomain);
> 
> Same comments as above.

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:378
> > +    auto& executionStack = ensureExecutionStackForRegistrableDomain(registrableDomain);
> 
> Same comments as above.

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:419
> > +    auto& statistics = ensureExecutionStackForRegistrableDomain(registrableDomain);
> 
> Same comments here as above -- you can use auto for the frameID and call
> RegistrableDomain { document.url() } in the function call.

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:430
> > +            documentWithScriptsIterator->value.set(activeScriptID, apis);
> 
> I think you can use WTFMove(apis) here and subsequent calls.

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:437
> > +        apisExecutingInScript.set(activeScriptID, apis);
> 
> Ditto, WTFMove(apis)

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:455
> > +    auto& statistics = ensureExecutionStackForRegistrableDomain(registrableDomain);
> 
> auto& statistics =
> ensureExecutionStackForRegistrableDomain(RegistrableDomain { document.url()
> });

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:456
> > +    String APIJSONString = statistics.toJSONString(frameID);
> 
> String APIJSONString = statistics.toJSONString(frameID {
> document.frameID()->toUInt64() });

Fixed.

[Keith Miller]

(In reply to Umar Iqbal from comment #8)
> (In reply to Keith Miller from comment #7)
> > Comment on attachment 406201 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=406201&action=review
> > 
> > > Source/JavaScriptCore/ChangeLog:8
> > > +        These hooks capture eval/Function, callbacks, and microtasks execution.
> > 
> > Can you add a note that this isn't going to ship in production. Otherwise,
> > this code looks very scary from a performance POV :P
> 
> Thanks for the feedback. I will update the change log to reflect that.

Thanks!

[John Wilander]

Comment on attachment 406201 [details]
Patch

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

Looks good to me. I trust Keith has reviewed the JSC parts which I'm not proficient in. I focused on the observer.

> Source/WebCore/dom/Document.h:639
> +        Optional<FrameIdentifier> frameID() const;

Double indent.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:288
> +        // We use HashMap.add here because there is a chance that script URL is none (in case of microtasks).

I typically put empty parenthesis after function names to make it obvious that they are functions. Here that would be HashMap.add().

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:289
> +        // HashMap.add will ensure that the exisitng script URL does not get overwritten.

Ditto.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:320
> +        return 0;

Is 0 guaranteed to mean "empty" here? Or do we risk having all scripts be identified as 0? A better way is to use Optional<> with WTF::nullopt representing the empty result.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:331
> +            auto temp = executingScriptsStackIterator->value.last();

Can we come up with a better variable name than temp?

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:344
> +    return 0;

Same thing for 0 here.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:350
> +        return 0;

Same thing for 0 here.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:364
> +    return 0;

Same thing for 0 here.

> Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:370
> +        return { };

Is an empty string the right thing to return here or could Optional<> work? This one doesn't seem as important though since it's just for text output. Not commenting further on returning empty strings. Doesn't seem like a big issue.

[Umar Iqbal]

Created attachment 406222 [details]
Patch

[Umar Iqbal]

Created attachment 406223 [details]
Patch

[Umar Iqbal]

(In reply to John Wilander from comment #11)
> Comment on attachment 406201 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406201&action=review
> 
> Looks good to me. I trust Keith has reviewed the JSC parts which I'm not
> proficient in. I focused on the observer.
> 
> > Source/WebCore/dom/Document.h:639
> > +        Optional<FrameIdentifier> frameID() const;
> 
> Double indent.

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:288
> > +        // We use HashMap.add here because there is a chance that script URL is none (in case of microtasks).
> 
> I typically put empty parenthesis after function names to make it obvious
> that they are functions. Here that would be HashMap.add().

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:289
> > +        // HashMap.add will ensure that the exisitng script URL does not get overwritten.
> 
> Ditto.

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:320
> > +        return 0;
> 
> Is 0 guaranteed to mean "empty" here? Or do we risk having all scripts be
> identified as 0? A better way is to use Optional<> with WTF::nullopt
> representing the empty result.

Thanks for the suggestion. I use Optional<> now.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:331
> > +            auto temp = executingScriptsStackIterator->value.last();
> 
> Can we come up with a better variable name than temp?
>

Changed name to scriptID.
 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:344
> > +    return 0;
> 
> Same thing for 0 here.

Fixed.

> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:350
> > +        return 0;
> 
> Same thing for 0 here.
> 

Fixed.

> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:364
> > +    return 0;
> 
> Same thing for 0 here.
> 

Fixed.

> > Source/WebKit/WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:370
> > +        return { };
> 
> Is an empty string the right thing to return here or could Optional<> work?
> This one doesn't seem as important though since it's just for text output.
> Not commenting further on returning empty strings. Doesn't seem like a big
> issue.


Using Optional<String> now.

[Radar WebKit Bug Importer]

<rdar://problem/66955639>