Bug 161264 – REGRESSION (r203535): Web Inspector: Inspector overlay node info has disappeared

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

In the DOM tree (or for any logged nodes), hovering them shows a highlight overlay for that node in the inspected page.  The highlight overlay still shows, but the information about the node (width, height, id, class, role, etc) has disappeared.

[Radar WebKit Bug Importer]

<rdar://problem/28035989>

[Joseph Pecoraro]

This appears to have broken with:
<https://trac.webkit.org/changeset/203535>

[Joseph Pecoraro]

Maybe this doesn't work anymore? We should audit InspectorOverlayPage.js for issues after that change:

    DOMBuilder.prototype.appendTextNode = function(content)
    {
        var node = document.createTextNode();
        node.textContent = content;
        this.element.appendChild(node);
        return node;
    }

[Matt Baker]

Regressed in https://trac.webkit.org/changeset/203535.

[Devin Rousso]

Created attachment 287166 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 287166 [details]
Patch

We should be able to test this by crashing on debug builds if there was any exception in the overlay page. We should never have exceptions evaluating something in the overlay page. Here is what we could do:

Source/WebCore/inspector/InspectorOverlay.cpp has a few "evaluateInOverlay*" methods that look like:

    overlayPage()->mainFrame().script().evaluate(ScriptSourceCode(makeString("dispatch(", command->toJSONString(), ')')));

This is using ScriptController's evaluate method which includes optional exception details:

    JSC::JSValue evaluate(const ScriptSourceCode&, ExceptionDetails* = nullptr);

We could pass in exception details and try to ASSERT that there was an exception. ASSERT(exceptionDetails.message.isNull());

Let me know what you decide to do, because ultimately we will want something like this.

[Joseph Pecoraro]

> We could pass in exception details and try to ASSERT that there was an
> exception. ASSERT(exceptionDetails.message.isNull());

Or even simpler (based off of WebPage.cpp's use of evaluate()):

    JSValue result = overlayPage()->mainFrame().script().evaluate(...);
    ASSERT_UNUSED(result, !result); // Should never have exceptions evaluating in the overlay page.

[Devin Rousso]

Created attachment 287180 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 287180 [details]
Patch

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

> Source/WebCore/inspector/InspectorOverlay.cpp:928
> +    ExceptionDetails exceptionDetails;
> +    page->mainFrame().script().evaluate(ScriptSourceCode(makeString("dispatch(", command->toJSONString(), ')')), &exceptionDetails);
> +    ASSERT(exceptionDetails.message.isNull()); // There should never be exceptions when evaluating in the overlay page.

Err instead of this could you just do the other assert?

    static void evaluateCommandInOverlay(Page* page, Ref<InspectorArray>&& command)
    {
        JSValue result = page->mainFrame().script().evaluate(ScriptSourceCode(makeString("dispatch(", command->toJSONString(), ')')));
        ASSERT_UNUSED(result, result); // There should never be exceptions when evaluating in the overlay page.
    }

[Devin Rousso]

Created attachment 287189 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 287189 [details]
Patch

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

r=me

> Source/WebCore/ChangeLog:8
> +        No new tests.  Fixing JavaScript error in Inspector overlay.

You could say that adding the ASSERT would cause tests to crash if this happens in the future.

[Devin Rousso]

Created attachment 287191 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 287191 [details]
Patch

Clearing flags on attachment: 287191

Committed r205068: <http://trac.webkit.org/changeset/205068>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.