370721 - Image dragged from the Desktop to the content area fails to render

Status: RESOLVED FIXED

(Camino Graveyard :: Page Layout, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Splitting the image-dragging variant of bug 350331 comment 6-14 off into its own bug (it can be fixed by backing out bug 345425) and moving pink's blocking camino1.1+.

Presumably a straight backout will do bad things to Firefox, so we need to find a solution that fixes bug 345425 and this bug, and then request blocking1.8.1.x ASAP afterwards.

+++ This bug was initially created as a clone of Bug #350331 +++

Comment #6 [reply] Mike Pinkerton  2006-09-25 07:34:38 PST

this is exactly the bug i was talking about the other day on irc where if i
dragged an image from the finder into a window, it would load, then dragging a
second one wouldn't display. resizing the window would fix it.

this should block, and shouldn't be too hard to walk backwards and find it.
it's been w/in the past few months, after spellcheck landed. I had a build
pretty close to when spellcheck landed that i was using for a while and I don't
think i recall seeing it, but as soon as i updated to a more recent version
(build from last week), I noticed it.

Comment #7 [reply] JK  2006-10-09 21:04:31 PST

I hope this helps...

Procedure:
1) Downloaded Camino branch (1.1) builds from:
http://ftp.mozilla.org/pub/mozilla.org/camino/nightly/
2) Mounted the DMG
3) Opened Camino (from the DMG)
4) Drag and drop an JPEG file from my Desktop to the empty window
5) If Camino failed to display the JPEG file, it was considered a bad build

First (oldest) build found to be bad: 2006-07-26-00-1.1-M1.8 (2006072600)
Both builds from 2006-07-24 were fine, and there was no build on 2006-07-25.

Comment #8 [reply] Smokey Ardisson (unreliable; no bugmail)  2006-10-09 21:38:30 PST

(In reply to comment #7)
> First (oldest) build found to be bad: 2006-07-26-00-1.1-M1.8 (2006072600)
> Both builds from 2006-07-24 were fine, and there was no build on 2006-07-25.

Branch regression range: http://tinyurl.com/rg965

Nothing there jumps out at me; it could conceivably be one of the security/js
bugs (or possibly the drag/drop change, but that wouldn't/shouldn't explain the
"regular pages fail to load" manifestation of the bug)....

Comment #9 [reply] philippe  2006-10-09 22:31:06 PST

On trunk, I have:
2006072217 (1.2+) works
2006072322 (1.2+) fails

maybe (not sure if that query is correct)
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=20060722+16%3A00%3A00&maxdate=20060723+22%3A00%3A00&cvsroot=%2Fcvsroot

244116 ??

Comment #10 [reply] Smokey Ardisson (unreliable; no bugmail)  2006-10-09 22:59:58 PST

Well, both branch and trunk ranges have bug 345425 (and that's the only common
bug), so that seems a possible culprit for at least the dragging-image version
(maybe the comment 0 flavor is different)?

Comment #11 [reply] Mike Beltzner  2006-10-10 10:21:41 PST

Not blocking 1.8.1 for this, but if you find out that it's something which we
do in the branch itself, please nominate for 1.8.1.1 ...

Comment #12 [reply] Torben  2006-10-11 00:46:31 PST

Backing out the patch from bug 345425 fixes this bug in my current trunk build
(at least the STR in comment 7, which is the only way I've ever seen this).

Comment #13 [reply] Smokey Ardisson (unreliable; no bugmail)  2006-10-11 13:26:34 PST

(In reply to comment #12)
> Backing out the patch from bug 345425 fixes this bug in my current trunk build
> (at least the STR in comment 7, which is the only way I've ever seen this).

Since I can't back out patches that don't back themselves out cleanly ;) I
can't test the other scenarios (which I see a lot, at least the image-as-page
not loading flavor), but were someone to post a not-for-checkin backout patch,
I'd be more than happy to give it a spin ;)

Comment #14 [reply] Torben  2006-10-11 23:47:08 PST

Created an attachment (id=242040) [details]
Backout bug 345425, for testing only

> were someone to post a not-for-checkin backout patch,
> I'd be more than happy to give it a spin

Comment #15 [reply] Smokey Ardisson (unreliable; no bugmail)  2006-10-14 12:08:35 PST

For a long time I wasn't able to repro the "pages fail to display" bug with the
backout patch (thanks torben!) in my trunk build, but I just reproduced it :(

So we're dealing with two different bugs, one in comment 0 - comment 5 (with no
solution, and hard to repro) and one in comment 6 and beyond (which is fixed by
backing out bug 345425).  Both are regressions.

Comment #16 [reply]

Comment 1

[Mark Mentovai]

Oh, OK.

Comment 2

[Stuart Morgan]

In debug builds, once in this state I see the following every time I focus the window until I resize or otherwise fix it:
###!!! ASSERTION: win is null.  this happens [often on xlib builds].  see bug #79213: 'Error', file [...]/mozilla/content/events/src/nsEventStateManager.cpp, line 846
The bug isn't very informative, but hopefully that will give us some idea where to start looking.

Comment 3

[Stuart Morgan]

It looks like we have a top-level, unchanging ChildView for each browser, which contains one ChildView that should change on each pageload. I set a breakpoint on [ChildView initWithFrame:geckoChild:eventSink:] and in the bad cases I see the new ChildView for the page (two actually; presumably nested) get created, but the child of the top-level ChildView doesn't change until resize, so the problem is definitely bubbling up to the ChildView level.

Comment 4

[Mark Mentovai]

Camino calls nsDragHelperService::Drop directly from CHBrowserView performDragOperation:, and nobody calls nsDragService::EndDragSession any longer.  nsDragHelperService::Drop used to, but that was wrong in Carbon widgets, because there would be another subsequent call to nsDragService::EndDragSession, and the first call would create a weird state so that the second call would do bad stuff.

Comment 5

[Mark Mentovai]

Attachment: Possible fix

This might do it.  In fact, it probably will do it, but I'm concerned about regressions in Firefox.  Stupid shared widget code.

Comment 6

[Samuel Sidler (old account; do not CC)]

Comment on attachment 259030 [details] [diff] [review]
Possible fix

Stuart, you said you could take a look at this. If we take it on the branch, we're going to ifdef it (or whatever) so only Camino gets it.

Comment 7

[Stuart Morgan]

Comment on attachment 259030 [details] [diff] [review]
Possible fix

Looks fine for branch. I guess this needs a respin for the #ifdef?

Comment 8

[Mark Mentovai]

Yeah, I'll toss some #ifdefs in there and request approval.

Comment 9

[Mark Mentovai]

Attachment: Fix with #ifdefs

This is a Cocoa-only (therefore Camino-only) change targeted for the 1.8 branch.  We'll at least need more testing on the trunk, because other products use Cocoa widgets there too, and we don't want to introduce any product-specific #ifdefs there.

Comment 10

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 261009 [details] [diff] [review]
Fix with #ifdefs

sr=pink

Comment 11

[Mark Mentovai]

Comment on attachment 261009 [details] [diff] [review]
Fix with #ifdefs

This is #ifdeffed to affect Camino only, nothing else uses Cocoa widgets on the 1.8[.1] branch.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 261009 [details] [diff] [review]
Fix with #ifdefs

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 13

[Mark Mentovai]

Checked in on the MOZILLA_1_8_BRANCH.  Leaving open because this is unfixed on the trunk, where further investigation will be needed because things are being Cocoaified there.

Comment 14

[Stuart Morgan]

Pushing to 2.0 for the remaining issue, since it's trunk-only at this point.

Comment 15

[Stuart Morgan]

This is WFM in recent trunk, so closing.

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

It bothers me that we landed stuff here and then closed the bug WFM (and also confused me when I stumbled on it looking for a related bug that I thought was this one), so I'm going to fix this up to be a FIXED bug on 1_8.