Closed Bug 285828 Opened 20 years ago Closed 20 years ago

[FIX]nsIWebPageDescriptor.loadPage returns different results on second load

Categories

(Core :: DOM: Navigation, defect, P1)

Product:
Core
Component:
DOM: Navigation
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.8beta2

People

(Reporter: jason.barnabe, Assigned: bzbarsky)

Details

Attachments

(2 files)

example code
1.27 KB, text/plain
Details
Proposed patch
2.72 KB, patch
Biesinger
: review+
jst
: superreview+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050303 Firefox/1.0+ Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050303 Firefox/1.0+ Per bug 172817 comment 70... Reproducible: Always Steps to Reproduce: 1. Call loadPage with DISPLAY_AS_SOURCE 2. Output the text nodes of the resulting document's pre elements. 3. Same as #1 4. Same as #2 Actual Results: The first time, the element names, attribute names, and attribute values all come out as null. The second time, they come out as their real values.
Attached file example code —
Add this code anywhere you have a page descriptor (pageCookie is a page descriptor). I put it at the start of BrowserViewSourceOfURL.
Do you need the "load 1" and "load 2" alerts to reproduce the bug?
Since loadPage runs asynchronously, normally you'd set up a progress listener to wait for it to end. To keep things simple, I instead added alerts to pause execution in the thread to give the loadPage thread chance to finish.
Ok... so the instructions are to wait a while on the alerts, right? Out of curiousity, on a simple page like this: <html> <body> aaa </body> </html> What's returned the first time? What's returned the second time?
(In reply to comment #4) > Ok... so the instructions are to wait a while on the alerts, right? loadPage is pretty quick if the page is in cache. So wait if you want, but it'll probably be done before you can even get your mouse to OK. > What's returned the first time? What's returned the second time? First time: <null> <null> aaa </null> </null> Second time: <html> <body> aaa </body> </html>
Attached patch Proposed patch — — Splinter Review
So the problem is that currently we actually modify the passed-in page descriptor when doing a view-source load. The solution is to clone it and modify the clone instead. The other change is to always null out the parent and set false the "is frame" flag, not just when we're doing view-source.
Assignee: adamlock → bzbarsky
Status: NEW → ASSIGNED
Attachment #179059 - Flags: superreview?(jst)
Attachment #179059 - Flags: review?(cbiesinger)
So to make it clear what's going on, without this patch the first load basically loads view-source:whateverURL while the second loads view-source:view-source:whateverURL That's why there are textnodes with nodeValues equal to the tagnames in the latter case...
OS: Windows XP → All
Priority: -- → P1
Hardware: PC → All
Summary: nsIWebPageDescriptor.loadPage returns different results on second load → [FIX]nsIWebPageDescriptor.loadPage returns different results on second load
Target Milestone: --- → mozilla1.8beta2
Comment on attachment 179059 [details] [diff] [review] Proposed patch sr=jst
Attachment #179059 - Flags: superreview?(jst) → superreview+
Comment on attachment 179059 [details] [diff] [review] Proposed patch + if (!shEntryIn) { return NS_ERROR_INVALID_POINTER; might this be better as NS_ERROR_INVALID_ARG? (yeah, I know you didn't change it. leave it as-is if you want) - // NULL out inappropriate cloned attributes... - shEntry->SetParent(nsnull); - shEntry->SetIsSubFrame(PR_FALSE); why are you moving this?
> might this be better as NS_ERROR_INVALID_ARG? Yes. Will do. > why are you moving this? Because that should be done in general, not just for view-source. Certainly the parent pointer should be cleared for any SHEntry objects that are not in the session history tree...
Comment on attachment 179059 [details] [diff] [review] Proposed patch ok, but why not do this directly after cloning, since the cloned entry is surely not in the sh tree either?
> ok, but why not do this directly after cloning Er... that's exactly what I do: rv = src->Clone(getter_AddRefs(dest)); // error check rv + // null out inappropriate cloned attributes... + dest->SetParent(nsnull);
Comment on attachment 179059 [details] [diff] [review] Proposed patch oh, ok... when I wrote that, I meant the other Clone call, but I now see why you're doing what you're doing. sorry for the confusion. r=biesi (hm... looks like this: sup = do_QueryInterface(dest); *aPageDescriptor = sup; NS_ADDREF(*aPageDescriptor); could be replaced with a single CallQueryInterface call)
Attachment #179059 - Flags: review?(cbiesinger) → review+
Fixed. Jason, thanks for finding this and for helping debug!
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: