Bug 56439

Created attachment 85896 [details] cleanup + minor crash fix

(In reply to comment #1) > Created an attachment (id=85896) [details] > cleanup + minor crash fix I included the bug fix because this was a relatively small patch but I'll split it if other people think it's better that way.

Comment on attachment 85896 [details] cleanup + minor crash fix View in context: https://bugs.webkit.org/attachment.cgi?id=85896&action=review > Source/WebCore/editing/ReplaceSelectionCommand.cpp:626 > + Node* blockquoteNode = (!context || isMailPasteAsQuotationNode(context)) ? context : enclosingNodeOfType(firstPositionInNode(context), isMailBlockquote, CanCrossEditingBoundary); Nit: It took me a while to figure out why you added !context (i.e., it's ok to pass NULL to isMailPasteAsQuotationNode, but it's not OK to pass NULL to firstPositionInNode). Maybe pull this out into a helper function (it could also be used in line 559). Alternately, maybe firstPositionInNode should return a null position when passed in a NULL pointer.

Comment on attachment 85896 [details] cleanup + minor crash fix View in context: https://bugs.webkit.org/attachment.cgi?id=85896&action=review >> Source/WebCore/editing/ReplaceSelectionCommand.cpp:626 >> + Node* blockquoteNode = (!context || isMailPasteAsQuotationNode(context)) ? context : enclosingNodeOfType(firstPositionInNode(context), isMailBlockquote, CanCrossEditingBoundary); > > Nit: It took me a while to figure out why you added !context (i.e., it's ok to pass NULL to isMailPasteAsQuotationNode, but it's not OK to pass NULL to firstPositionInNode). Maybe pull this out into a helper function (it could also be used in line 559). Alternately, maybe firstPositionInNode should return a null position when passed in a NULL pointer. Extracting a helper function sounds good. isMailPasteAsQuotationOrMailBlockquoteDescendent?

Comment on attachment 85896 [details] cleanup + minor crash fix View in context: https://bugs.webkit.org/attachment.cgi?id=85896&action=review >>> Source/WebCore/editing/ReplaceSelectionCommand.cpp:626 >>> + Node* blockquoteNode = (!context || isMailPasteAsQuotationNode(context)) ? context : enclosingNodeOfType(firstPositionInNode(context), isMailBlockquote, CanCrossEditingBoundary); >> >> Nit: It took me a while to figure out why you added !context (i.e., it's ok to pass NULL to isMailPasteAsQuotationNode, but it's not OK to pass NULL to firstPositionInNode). Maybe pull this out into a helper function (it could also be used in line 559). Alternately, maybe firstPositionInNode should return a null position when passed in a NULL pointer. > > Extracting a helper function sounds good. isMailPasteAsQuotationOrMailBlockquoteDescendent? Sure. I was actually just thinking of pulling out enclosingNodeOfType(firstPositionInNode(context), isMailBlockquote, CanCrossEditingBoundary) with a NULL check (nearestParentMailBlockquote?). But that works too.

On my second thought, my initial proposal doesn't work and adding a null check to firstPositionInNode/lastPositionInNode makes most sense now. Will be uploading a patch soon.

Created attachment 85984 [details] Added a null pointer check to firstPositionInOrBeforeNode/lastPositionInOrAfterNode

Comment on attachment 85984 [details] Added a null pointer check to firstPositionInOrBeforeNode/lastPositionInOrAfterNode View in context: https://bugs.webkit.org/attachment.cgi?id=85984&action=review > Source/WebCore/editing/htmlediting.h:118 > + return node && editingIgnoresContent(node) ? positionBeforeNode(node) : firstPositionInNode(node); This doesn't look right. && has precedence over ?, so if |node| is NULL, we return firstPositionInNode(NULL). That seems safe, but in lastPositionInOrAfterNode, we return lastPositionInNode(NULL), which calls lastOffsetInNode(NULL), which will try to dereference NULL.

Comment on attachment 85984 [details] Added a null pointer check to firstPositionInOrBeforeNode/lastPositionInOrAfterNode View in context: https://bugs.webkit.org/attachment.cgi?id=85984&action=review >> Source/WebCore/editing/htmlediting.h:118 >> + return node && editingIgnoresContent(node) ? positionBeforeNode(node) : firstPositionInNode(node); > > This doesn't look right. && has precedence over ?, so if |node| is NULL, we return firstPositionInNode(NULL). That seems safe, but in lastPositionInOrAfterNode, we return lastPositionInNode(NULL), which calls lastOffsetInNode(NULL), which will try to dereference NULL. Ah that's a good point. I should just do "!node ||"

Created attachment 85992 [details] Added a null pointer check to firstPositionInOrBeforeNode/lastPositionInOrAfterNode

Thanks for the review! Landing it now.

Committed r81295: <http://trac.webkit.org/changeset/81295>