[webkit-reviews] review denied: [Bug 127269] A few utility functions for setting/clearing the op_debug hasBreakpointFlag. : [Attachment 221628] the patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 20 09:29:24 PST 2014


Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 127269: A few utility functions for setting/clearing the op_debug
hasBreakpointFlag.
https://bugs.webkit.org/show_bug.cgi?id=127269

Attachment 221628: the patch.
https://bugs.webkit.org/attachment.cgi?id=221628&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=221628&action=review


> Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp:406
> +void UnlinkedCodeBlock::clearAllBreakpointsIn(RefCountedArray<Instruction>&
instructions)
> +{
> +    LineColumnInfoList& list = opDebugLineColumnInfoList();
> +    for (LineColumnInfoList::iterator it = list.begin(); it != list.end();
++it) {
> +	   static const int hasBreakpointFlagOffset = 2; 
> +	   unsigned offset = it->instructionOffset;
> +	   ASSERT(m_vm->interpreter->getOpcodeID(instructions[offset].u.opcode)
== op_debug);
> +	   instructions[offset + hasBreakpointFlagOffset] = false;
> +    }
> +}

This doesn't look right. The unlinked CodeBlock should never contain
breakpoints, or metadata about whether they're set. The unlinked CodeBlock is
cached and shared among webpages. Putting breakpoint metadata in it would cause
debugging in one webpage to set breakpoints in another, and it would cause
breakpoints to persist across page loads.


More information about the webkit-reviews mailing list