Bug 93884 – Change behavior of MasqueradesAsUndefined to better accommodate DFG changes

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

With some upcoming changes to the DFG to remove uses of ClassInfo, we will be changing the behavior of MasqueradesAsUndefined. In order to make this change consistent across all of our execution engines, we will make this change to MasqueradesAsUndefined as a separate patch. After this patch, MasqueradesAsUndefined objects will only masquerade as undefined in their original context (i.e. their original JSGlobalObject). For example, if an object that masquerades as undefined in frame A is passed to frame B, it will not masquerade as undefined within frame B, but it will continue to masquerade in frame A.

[Mark Hahnenberg]

Created attachment 158088 [details]
Patch

[Early Warning System Bot]

Comment on attachment 158088 [details]
Patch

Attachment 158088 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13491297

[Early Warning System Bot]

Comment on attachment 158088 [details]
Patch

Attachment 158088 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13481943

[Gyuyoung Kim]

Comment on attachment 158088 [details]
Patch

Attachment 158088 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13490250

[Geoffrey Garen]

Comment on attachment 158088 [details]
Patch

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

r- because the builders are angry.

Please check my performance comment below, as well.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:645
> +    JITCompiler::Jump isMasqueradesAsUndefined = m_jit.branchTest8(JITCompiler::NonZero, JITCompiler::Address(resultPayloadGPR, Structure::typeInfoFlagsOffset()), JITCompiler::TrustedImm32(MasqueradesAsUndefined));
>      
> +    if (invert)
> +        m_jit.move(TrustedImm32(0), resultPayloadGPR);
> +    else
> +        m_jit.move(TrustedImm32(1), resultPayloadGPR);
> +
> +    JITCompiler::Jump notMasqueradesAsUndefined = m_jit.jump();
> +
> +    isMasqueradesAsUndefined.link(&m_jit);
> +    GPRTemporary localGlobalObject(this);
> +    GPRTemporary remoteGlobalObject(this);
> +    GPRReg localGlobalObjectGPR = localGlobalObject.gpr();
> +    GPRReg remoteGlobalObjectGPR = remoteGlobalObject.gpr();
> +    m_jit.move(JITCompiler::TrustedImmPtr(m_jit.graph().globalObjectFor(m_jit.graph()[operand].codeOrigin)), localGlobalObjectGPR);
> +    m_jit.loadPtr(JITCompiler::Address(resultPayloadGPR, Structure::globalObjectOffset()), remoteGlobalObjectGPR);
> +    m_jit.compare32(invert ? JITCompiler::NotEqual : JITCompiler::Equal, localGlobalObjectGPR, remoteGlobalObjectGPR, resultPayloadGPR);
> + 

This looks like it would be a performance regression. Do you have benchmark results for this patch?

I thought our plan was:
(a) Don't test masqueradesAsUndefined at all if you global object has not produced document.all
(b) Watchpoint equality comparisons, and fire the watchpoint if your global object does produce document.all

I believe that plan would be a speedup over what we have now.

> Source/JavaScriptCore/runtime/Structure.h:513
> +    inline bool Structure::isMasqueradesAsUndefined(JSGlobalObject* lexicalGlobalObject)
> +    {
> +        return typeInfo().masqueradesAsUndefined() && globalObject() == lexicalGlobalObject;
> +    }

I think just plain "masqueradesAsUndefined" would be a better name here.

[Mark Hahnenberg]

Created attachment 158150 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 158150 [details]
Patch

Attachment 158150 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13494290

[WebKit Review Bot]

Comment on attachment 158150 [details]
Patch

Attachment 158150 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13492326

New failing tests:
fast/js/document-all-between-frames.html

[WebKit Review Bot]

Created attachment 158175 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 158150 [details]
Patch

Attachment 158150 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13490366

[Mark Hahnenberg]

Created attachment 158448 [details]
Patch

[Mark Hahnenberg]

I'm not sure how to fix the EFL build failure. The compiler claims it's an incorrect use of JSValue::toBoolean without the ExecState* argument, but I don't see any incorrect uses of JSValue::toBoolean() anywhere in the entire Source directory. Any EFL people out there that can give me a hint?

[Geoffrey Garen]

Comment on attachment 158448 [details]
Patch

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

r=me with one repeated comment below.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:616
> +    m_jit.graph().globalObjectFor(m_jit.graph()[operand].codeOrigin)->masqueradesAsUndefinedWatchpoint()->add(speculationWatchpoint());

You should only do this if the watchpoint is still valid. If it's invalid, adding to it doesn't make sense.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:670
> +    m_jit.graph().globalObjectFor(m_jit.graph()[operand].codeOrigin)->masqueradesAsUndefinedWatchpoint()->add(speculationWatchpoint());

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp:3817
> +        m_jit.graph().globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->add(speculationWatchpoint());

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:581
> +    m_jit.graph().globalObjectFor(m_jit.graph()[operand].codeOrigin)->masqueradesAsUndefinedWatchpoint()->add(speculationWatchpoint());

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:635
> +    m_jit.graph().globalObjectFor(m_jit.graph()[operand].codeOrigin)->masqueradesAsUndefinedWatchpoint()->add(speculationWatchpoint());

Ditto.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp:3808
> +        m_jit.graph().globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->add(speculationWatchpoint());

Ditto.

[WebKit Review Bot]

Comment on attachment 158448 [details]
Patch

Attachment 158448 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13502484

New failing tests:
fast/js/document-all-between-frames.html

[WebKit Review Bot]

Created attachment 158457 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Gyuyoung Kim]

Comment on attachment 158448 [details]
Patch

Attachment 158448 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13500501

[Mark Hahnenberg]

Committed r125687: <http://trac.webkit.org/changeset/125687>

[Csaba Osztrogonác]

(In reply to comment #17)
> Committed r125687: <http://trac.webkit.org/changeset/125687>

It broke the whole world: https://bugs.webkit.org/show_bug.cgi?id=94144

Could you check it, please?

[WebKit Review Bot]

Re-opened since this is blocked by 94147

[Mark Hahnenberg]

Created attachment 160247 [details]
Patch

[Geoffrey Garen]

Comment on attachment 160247 [details]
Patch

r=me

[Filip Pizlo]

Comment on attachment 160247 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:788
> -                    || !isFinalObjectOrOtherSpeculation(forNode(node.child2()).m_type));
> +                    || !isFinalObjectOrOtherSpeculation(forNode(node.child2()).m_type)
> +                    || m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid());
>                  forNode(node.child1()).filter(SpecFinalObject);
>                  forNode(node.child2()).filter(SpecFinalObject | SpecOther);
>                  break;
>              } else if (right.shouldSpeculateFinalObject() && left.shouldSpeculateFinalObjectOrOther()) {
>                  node.setCanExit(
>                      !isFinalObjectOrOtherSpeculation(forNode(node.child1()).m_type)
> -                    || !isFinalObjectSpeculation(forNode(node.child2()).m_type));
> +                    || !isFinalObjectSpeculation(forNode(node.child2()).m_type)
> +                    || m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid());
>                  forNode(node.child1()).filter(SpecFinalObject | SpecOther);
>                  forNode(node.child2()).filter(SpecFinalObject);
>                  break;
>              } else if (left.shouldSpeculateArray() && right.shouldSpeculateArrayOrOther()) {
>                  node.setCanExit(
>                      !isArraySpeculation(forNode(node.child1()).m_type)
> -                    || !isArrayOrOtherSpeculation(forNode(node.child2()).m_type));
> +                    || !isArrayOrOtherSpeculation(forNode(node.child2()).m_type)
> +                    || m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid());
>                  forNode(node.child1()).filter(SpecArray);
>                  forNode(node.child2()).filter(SpecArray | SpecOther);
>                  break;
>              } else if (right.shouldSpeculateArray() && left.shouldSpeculateArrayOrOther()) {
>                  node.setCanExit(
>                      !isArrayOrOtherSpeculation(forNode(node.child1()).m_type)
> -                    || !isArraySpeculation(forNode(node.child2()).m_type));
> +                    || !isArraySpeculation(forNode(node.child2()).m_type)
> +                    || m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid());

Why did you change this?  Your patch doesn't change any of the backend code that corresponds to this!

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:796
> +                node.setCanExit(
> +                    !isAnySpeculation(forNode(node.child1()).m_type)
> +                    || !isAnySpeculation(forNode(node.child2()).m_type)
> +                    || m_codeBlock->globalObjectFor(node.codeOrigin)->masqueradesAsUndefinedWatchpoint()->isStillValid());

isAnySpeculation returns true all the time.  So this code is very strange.

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:798
> +                forNode(node.child1()).filter(SpecTop);
> +                forNode(node.child2()).filter(SpecTop);

Please remove this; filtering on TOP does nothing.

[Mark Hahnenberg]

Created attachment 160269 [details]
Oops

[Mark Hahnenberg]

Committed r126494: <http://trac.webkit.org/changeset/126494>

[Ryosuke Niwa]

It appears that this broke EFL builds:
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815

http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815/steps/compile-webkit/logs/stdio

[Mark Hahnenberg]

(In reply to comment #25)
> It appears that this broke EFL builds:
> http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815
> 
> http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815/steps/compile-webkit/logs/stdio

I mentioned earlier in the bug that I could not figure out how to fix these build errors. I see no mention of the functions that claim they don't compile anywhere in the Source directory.

[Ryosuke Niwa]

(In reply to comment #26)
> (In reply to comment #25)
> > It appears that this broke EFL builds:
> > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815
> > 
> > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815/steps/compile-webkit/logs/stdio
> 
> I mentioned earlier in the bug that I could not figure out how to fix these build errors. I see no mention of the functions that claim they don't compile anywhere in the Source directory.

It's in derived source.

[Filip Pizlo]

(In reply to comment #27)
> (In reply to comment #26)
> > (In reply to comment #25)
> > > It appears that this broke EFL builds:
> > > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815
> > > 
> > > http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug/builds/4815/steps/compile-webkit/logs/stdio
> > 
> > I mentioned earlier in the bug that I could not figure out how to fix these build errors. I see no mention of the functions that claim they don't compile anywhere in the Source directory.
> 
> It's in derived source.

I think Mark's point is that he already changed the code generators to use the right function signature.

So EFL may be doing something strange here.

[Ryosuke Niwa]

(In reply to comment #28)
> I think Mark's point is that he already changed the code generators to use the right function signature.
> 
> So EFL may be doing something strange here.

Maybe it just need a kick?

[Ryosuke Niwa]

It appears that touching the idl file worked:
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug

[Mark Hahnenberg]

(In reply to comment #30)
> It appears that touching the idl file worked:
> http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug

Sweet, thanks Ryosuke!