Closed Bug 1090616 Opened 5 years ago Closed 5 years ago

Crash when pasting image

Categories

(Core :: DOM: Editor, defect, critical)

32 Branch
x86
All
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
firefox33 --- wontfix
firefox34 + verified
firefox35 --- fixed
firefox36 --- verified

People

(Reporter: davidj, Assigned: bzbarsky)

References

Details

(Keywords: crash, crashreportid, regression)

Crash Data

Attachments

(2 files)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.111 Safari/537.36

Steps to reproduce:

- copy an image (but not a link) from a website into the clipboard
- go to http://www.groupworld.net/js/test.html
- enter any name in the username box
- don't bother enabling audio/video
- click on the whiteboard
- press cmd+V


Actual results:

Firefox crashes if you're using OS X, but it works fine on Windows 7.


Expected results:

Should either paste the image onto the whiteboard (if it originates on the same website), or print a security exception to the javascript console if the image is from another website.
Please post the crash id from about:crashes
- https://developer.mozilla.org/en-US/docs/How_to_get_a_stacktrace_for_a_bug_report
Flags: needinfo?(davidj)
crash id: 3d17ab36-21b9-41c6-b099-df60b2141029

Also note that it sometimes crashes in a slightly different method:

crash id: 12f84f0a-d24f-4d1d-87d9-2c7462141028

So it looks like the issue is either in nsHTMLEditor::DoInsertHTMLWithContext or somewhere above it.
Flags: needinfo?(davidj)
bp-3d17ab36-21b9-41c6-b099-df60b2141029
bp-12f84f0a-d24f-4d1d-87d9-2c7462141028
Severity: normal → critical
Crash Signature: [@ nsWSRunObject::GetWSNodes() ] [@ nsTextEditUtils::IsBreak(nsINode*) ]
Component: General → Editor
Keywords: crash, crashreportid
CR FF36 Win 7:
https://crash-stats.mozilla.com/report/index/ebb89c2c-6a82-4463-99a6-82c5d2141029

Regression range:
good=2014-05-06
bad=2014-05-07
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=81651ad5e43c&tochange=5bbc85136202

Suspected bug: bug 1003808 or bug 1003894.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(ayg)
Keywords: regression
Version: 33 Branch → 32 Branch
If I understand the crash report correctly, the crash is an access violation on this line, right?

http://hg.mozilla.org/mozilla-central/annotate/a255a234946e/editor/libeditor/nsWSRunObject.cpp#l631

If so, the only interesting thing that line does (I think) is call a virtual method of mNode.  It's not very likely that the vtable pointer got corrupted or anything like that, is it?

I don't think I can really debug this further without reproduction instructions that work on Linux, or a more precise regression range.  If anyone else can reproduce it, could they maybe try bisecting it to get an exact revision?
Flags: needinfo?(ayg)
Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad3657ba226
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140506031445
Crash:
https://hg.mozilla.org/integration/mozilla-inbound/rev/209d6ee29737
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 ID:20140506043145
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1ad3657ba226&tochange=209d6ee29737



Last Good: af3a7489a11f
First Bad: b090934d5f7f

Triggered by:
	b090934d5f7f	Aryeh Gregor — Bug 1003808 part 6 - Convert nsWSRunObject members to nsINode; r=ehsan
Blocks: 1003808
OS: Mac OS X → All
I can reproduce with STR of comment#0 on Ubuntu.  bp-001b5e48-621e-40a5-a578-861552141029
Flags: needinfo?(ayg)
I'll look at this tomorrow night if I have time.  I'm not really working until March, so no guarantees that I'll have time to deal with it until then.
Flags: needinfo?(ayg)
I can make Mozilla/5.0 (Windows NT 6.1; WOW64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141031030201 CSet: e0b505a37b1c crash by attempting to paste an image on the clipboard to a Webogram(1) conversation.

Report ID 	Date Submitted
bp-dec120a1-2ac6-4ed5-8e9c-3e6362141031	31/10/2014	01:21 p.m.
bp-7fd8a08a-909c-4595-a7db-c66a92141031	31/10/2014	01:16 p.m.

1 http://zhukov.github.io/webogram/#/im
I am thoroughly confused.  At the end of nsINode::InsertBefore, the node has a parent.  In its caller, InsertNodeTxn::DoTransaction, suddenly it does not:

(gdb) p newContent
$13 = (mozilla::dom::HTMLImageElement *) 0x7ffeb8ac3230
(gdb) p newContent->mParent
$14 = (mozilla::dom::HTMLDivElement *) 0x7fffca9ca1a0
(gdb) n
2215      return result;
(gdb) p result
$15 = (mozilla::dom::HTMLImageElement *) 0x7ffeb8ac3230
(gdb) n
2216    }
(gdb) n
nsINode::InsertBefore (this=0x7fffca9ca1a0, aNode=..., aChild=0x0, aError=...)
    at /mnt/ssd/checkouts/central/dom/base/nsINode.h:1601
1601      }
(gdb) n
mozilla::dom::InsertNodeTxn::DoTransaction (this=0x7fffc5461600)
    at /mnt/ssd/checkouts/central/editor/libeditor/InsertNodeTxn.cpp:64
64        NS_ENSURE_SUCCESS(rv.ErrorCode(), rv.ErrorCode());
(gdb) n
67        if (mEditor.GetShouldTxnSetSelection()) {
(gdb) p mNode->mParent
$16 = (nsINode *) 0x0
(gdb) p mNode
$17 = {mRawPtr = 0x7ffeb8ac3230}

See how $13 is the same as $17, and $14 (the parent of $13) is not null, but then $16 (the parent of $17) is null.  How on earth could that happen in the intervening lines?  The only code that was executed is a return statement, and an NS_ENSURE_SUCCESS, unless stepping through with "n" is missing something here.

I can't look into this further until March or April, but I'll leave it on my list of things to do then.
Not sure if it's relevant to comment 10 or not, but here is the code we use for firefox:

http://pastebin.com/CwBZ0cdy

Basically as soon as the image is pasted into our div, we copy it and then delete the image from the div.
> How on earth could that happen in the intervening lines? 

Like this:

#31 0x00000001059d1202 in nsINode::DispatchEvent (this=0x12e932700, aEvent=0x118f892c0, aRetVal=0x7fff5fbf8a77) at nsINode.cpp:1272
#32 0x0000000106860ba1 in mozilla::AsyncEventDispatcher::Run (this=0x118f2fb00) at AsyncEventDispatcher.cpp:57
#33 0x00000001056d833e in nsContentUtils::RemoveScriptBlocker () at ../../../mozilla/dom/base/nsContentUtils.cpp:5027
#34 0x00000001059235e0 in nsDocument::EndUpdate (this=0x12029e000, aUpdateType=1) at nsDocument.cpp:4791
#35 0x0000000106a59bd9 in nsHTMLDocument::EndUpdate (this=0x12029e000, aUpdateType=1) at nsHTMLDocument.cpp:2490
#36 0x00000001056ec50c in mozAutoDocUpdate::~mozAutoDocUpdate (this=0x7fff5fbf8d70) at mozAutoDocUpdate.h:38
#37 0x00000001056e4335 in mozAutoDocUpdate::~mozAutoDocUpdate (this=0x7fff5fbf8d70) at mozAutoDocUpdate.h:36
#38 0x00000001059d48db in nsINode::ReplaceOrInsertBefore (this=0x12e932700, aReplace=false, aNewChild=0x11895a680, aRefChild=0x0, aError=@0x7fff5fbf9198) at nsINode.cpp:2216

That's a mutation event firing.  In this case, the event listener is on the page and it sets .innerHTML to the empty string on the HTMLDivElement we're pasting into.  The relevant bit of script is in http://www.groupworld.net/js/new_whiteboard.min.js on line 1176 and looks like this:

 document.getElementById('paste_ff').addEventListener('DOMSubtreeModified',function() {
   if (pasteCatcher.children.length==1) {
     var img=pasteCatcher.firstElementChild.src;
     var img2=new Image();
     img2.onload=function() {
       new_whiteboard.paste_image(img2);
     }
     img2.src=img;
     pasteCatcher.innerHTML='';
   }
 },false);

I guess comment 11 would have saved me the trouble of looking through the minified source if I'd read it carefully...

Ehsan, do you have time to look into why this started crashing, exactly?  Did we remove a null-check somewhere here?
Flags: needinfo?(ehsan.akhgari)
Duplicate of this bug: 1094866
Crash Signature: [@ nsWSRunObject::GetWSNodes() ] [@ nsTextEditUtils::IsBreak(nsINode*) ] → [@ nsWSRunObject::GetWSNodes() ] [@ nsTextEditUtils::IsBreak(nsINode*) ] [@ nsINode::GetAsText() ]
Flags: needinfo?(bzbarsky)
So before the patches for bug 1003808 we hit a debug assertion in nsWSRunObject::PriorVisibleNode: aNode was null.  But with that assert commented out we hit a debug assertion in nsWSRunObject::FindRun: aNode is null.  If I comment _that_ assert out as well, I get an exception on the site:

JavaScript error: http://www.groupworld.net/js/new_whiteboard.min.js, line 1155: SecurityError: The operation is insecure.

but no crash.

So there was a preexisting problem here in terms of invariants being violated, but we managed to squeak through somehow.

Bisecting with those asserts commented out, the crash first appeared with:

changeset:   181720:b090934d5f7f
user:        Aryeh Gregor <ayg@aryeh.name>
date:        Fri May 02 14:11:26 2014 +0300
summary:     Bug 1003808 part 6 - Convert nsWSRunObject members to nsINode; r=ehsan

which is not terribly surprising.
And most immediately, we have:

3472 nsEditor::IsTextNode(nsIDOMNode *aNode)
3473 {
3474   if (!aNode)
3475   {
3476     NS_NOTREACHED("null node passed to IsTextNode()");
3477     return false;
3478   }

vs

3486 nsEditor::IsTextNode(nsINode *aNode)
3487 {
3488   return aNode->NodeType() == nsIDOMNode::TEXT_NODE;

and we call this on mNode, which changed type.  Without the bug 1003808 patches, I do in fact hit that NS_NOTREACHED once I comment out the fatal asserts.  So that's our null-check that went away...
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(ehsan.akhgari)
Flags: needinfo?(bzbarsky)
Comment on attachment 8520235 [details] [diff] [review]
Don't assume that the nodes we just pasted are still in the DOM, because mutation listeners suck like that

This patch applies to Aurora as-is.

Approval Request Comment

[Feature/regressing bug #]: 1003808
[User impact if declined]: Crashes on some sites
[Describe test coverage new/current, TBPL]: Probably not so great
[Risks and why]: I think risk is fairly low; the patch just adds a null-check
   on a codepath that otherwise crashes.
[String/UUID change made/needed]: None.
Attachment #8520235 - Flags: approval-mozilla-aurora?
Attachment #8520394 - Flags: approval-mozilla-beta?
Comment on attachment 8520235 [details] [diff] [review]
Don't assume that the nodes we just pasted are still in the DOM, because mutation listeners suck like that

Ok, so we have
if (!selNode)
  selNode = lastInsertNode; 
for other case, but not here.

I still don't know where you get nsEditor::IsTextNode crash, but the patch
should fix https://crash-stats.mozilla.com/report/index/3d17ab36-21b9-41c6-b099-df60b2141029 since this way we don't pass null to nsWSRunObject::nsWSRunObject.
But why didn't we used to crash in nsWSRunObject::GetWSNodes() which nsWSRunObject::nsWSRunObject calls.

I'm missing some code path here.
> I still don't know where you get nsEditor::IsTextNode crash

In builds with some but not all of the patches for bug 1003808, sorry.  Specifically, part 6 in that bug introduced the crash while still using IsTextNode, while part 9 "cleaned up" things to crash directly by doing mNode->NodeType() == nsIDOMNode::TEXT_NODE, and there have been more patches since then to GetWSNodes that minorly rearrange that line.

You're right that on tip without my patch we just crash in GetWSNodes directly, when dereferencing mNode.
Comment on attachment 8520235 [details] [diff] [review]
Don't assume that the nodes we just pasted are still in the DOM, because mutation listeners suck like that

I see. r=me
Attachment #8520235 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/3686b4a6907e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8520394 [details] [diff] [review]
Patch for beta

Beta+
Attachment #8520394 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8520235 [details] [diff] [review]
Don't assume that the nodes we just pasted are still in the DOM, because mutation listeners suck like that

Aurora+
Attachment #8520235 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Reproduced the crash on Windows 7 x64 with scenario from comment 0 and Firefox 34 beta 8. The issue no longer reproduces with Firefox 34 Beta 9, as the image is correctly pasted in the editor.
QA Whiteboard: [good first verify]
ubuntu 14.10 linux

tested on more than on build of firefox, on developer edition 37.0a2 (2015-01-27) also 36 beta 7. It does not crash, but does nothing at all using this test case. There is nothing pasted to the whiteboard I only get the console response of 
SecurityError: The operation is insecure.
(In reply to charja13 from comment #28)
> ubuntu 14.10 linux
> 
> tested on more than on build of firefox, on developer edition 37.0a2
> (2015-01-27) also 36 beta 7. It does not crash, but does nothing at all
> using this test case. There is nothing pasted to the whiteboard I only get
> the console response of 
> SecurityError: The operation is insecure.

This is actually expected. Expected results from comment 0 state: "Should either paste the image onto the whiteboard (if it originates on the same website), or print a security exception to the javascript console if the image is from another website.". I suspect you used an image from another site.

You can try copying an image from http://groupworld.net/ and try to paste that. That should work fine.
(In reply to Florin Mezei, QA (:FlorinMezei) from comment #29)
> (In reply to charja13 from comment #28)
> > ubuntu 14.10 linux
> > 
> > tested on more than on build of firefox, on developer edition 37.0a2
> > (2015-01-27) also 36 beta 7. It does not crash, but does nothing at all
> > using this test case. There is nothing pasted to the whiteboard I only get
> > the console response of 
> > SecurityError: The operation is insecure.
> 
> This is actually expected. Expected results from comment 0 state: "Should
> either paste the image onto the whiteboard (if it originates on the same
> website), or print a security exception to the javascript console if the
> image is from another website.". I suspect you used an image from another
> site.
> 
> You can try copying an image from http://groupworld.net/ and try to paste
> that. That should work fine.

okay, Thank you. I did it that way and it pasted fine with no problems once so ever.
Works fine on linux and both browsers then.
Reproduced the crash on Firefox 36 Nightly (build id: 20141028030204) on Ubuntu 12.04, Mac OS X 10.8 and Windows 7 x64. On Firefox 36 Beta 7 the image is not pasted onto the whiteboard if it's from another website and a security exception is thrown in the browser console: "SecurityError: The operation is insecure. new_whiteboard.min.js:1158"; the image is pasted if it's from http://groupworld.net/, so verified as fixed on 36 Beta 7 (build id: 20150205114429) under Ubuntu 12.04 64 bit, Mac OS X 10.8 and Windows 7 x64.
Status: RESOLVED → VERIFIED
Yes, that security error is expected. That should hopefully be resolved once issue 891247 is fixed.
You need to log in before you can comment on or make changes to this bug.