Closed Bug 285828 Opened 15 years ago Closed 15 years ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.8beta2

People

(Reporter: jason.barnabe, Assigned: bzbarsky)

Details

Attachments

(2 files)

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 patchSplinter 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: 15 years ago
Resolution: --- → FIXED
Verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.