Closed Bug 321837 Opened 17 years ago Closed 17 years ago

nsWebBrowserFind::FindNext fails to wrap when searching chrome docshells


(Core :: DOM: Navigation, defect)

Not set





(Reporter: bugzilla-mozilla-20000923, Assigned: adamlock)




(2 files)

This came up because ChatZilla is now using a type=chrome <browser> for its chat area, and (among other things that got broken) searching refuses to wrap at all. This turns out to be because the web browser find code enumerates only type=content docshells when wrapping/searching frames.

Personally, that seems rather wrong (as the root search frame stops it escaping into the browser UI /anyway/), but I'm going to leave that. My patch just adds an extra check at the end, so that if you call FindNext on a chrome docshell and it fails all the usual wrapping attempts, it wraps within itself anyway.

This is required to make wrapping find work in ChatZilla. It has no effect on FAYT or the find dialog in the browser (any of them), as they all search content docshells.
Darin, Gavin - who should be pegged to review?
Blocks: 306108
I might as well follow up on bug 306108 comment 57 I guess.
Flags: blocking1.9a1?
Attachment #207126 - Flags: review?(rbs)
Comment on attachment 207126 [details] [diff] [review]
Do a last-ditch wrap for chrome docshells

> as the root search frame stops it escaping into the browser UI /anyway/

Mindful of that, try to just init the web browser find code enumerator with nsIDocShellTreeItem::typeAll, and see how it goes. I am not very enthusiastic about your band-aid patch as it stands, but could be persuaded if there isn't any other cleaner/clearer way.
This fixes find wrapping in ChatZilla, and has been confirmed to not affect find in Firefox or SeaMonkey.
At the same time, it'd be nice to get this fixed in, so I'd rather Silver's patch was taken if it's safer.
Attachment #215196 - Flags: review?(rbs)
Comment on attachment 215196 [details] [diff] [review]
Use typeAll instead

Attachment #215196 - Flags: review?(rbs) → review+
Attachment #207126 - Flags: review?(rbs) → review-
Attachment #215196 - Flags: superreview?(darin)
Comment on attachment 215196 [details] [diff] [review]
Use typeAll instead

This makes sense to me, but I think it would be good to get Simon's input since he wrote the code that is being changed.
Attachment #215196 - Flags: review?(sfraser_bugs)
I'm worried about this change because we've had security bugs in the past filed where someone uses Find to get at the contents of a different browser.

With this change, will Find in 'content' ever propagate to 'chrome'?
No, because it only enumerates children of the starting docshell, and content can't contain chrome.
Simon: It'd be great to get your thoughts on this patch again given comment #8.  Thanks!
Attachment #215196 - Flags: review?(sfraser_bugs) → review+
Attachment #215196 - Flags: superreview?(darin) → superreview?(bzbarsky)
Comment on attachment 215196 [details] [diff] [review]
Use typeAll instead

I think rbs's r= here can count as sr, but if we need the extra stamp.... ;)

I can't land anything real until at least Sunday, but if this still needs to be checked in at that point, please let me know.
Attachment #215196 - Flags: superreview?(bzbarsky) → superreview+
Closed: 17 years ago
Resolution: --- → FIXED
I'd like to have this on branch(es), who should I request 1.8.1 approval from?
Flags: blocking1.9a1?
Comment on attachment 215196 [details] [diff] [review]
Use typeAll instead

Picking randomly.
Attachment #215196 - Flags: approval-branch-1.8.1?(bzbarsky)
I'm really not sure what the safety vs benefit tradeoffs are here (so basically, I don't know that I'm qualified to evaluate this for approval; if I do evaluate it, I'd go with not taking this on branch).
Comment on attachment 215196 [details] [diff] [review]
Use typeAll instead

I'm not comfortable taking this on the branch, frankly....
Attachment #215196 - Flags: approval-branch-1.8.1?(bzbarsky) → approval-branch-1.8.1-
You need to log in before you can comment on or make changes to this bug.