Closed Bug 617789 Opened 14 years ago Closed 10 years ago

Links in "view page source" are incorrect when base href exists

Categories

(Core :: DOM: HTML Parser, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla35

People

(Reporter: pscott, Assigned: dij, Mentored)

References

Details

(Whiteboard: [good first bug][lang=C++])

Attachments

(1 file, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.12) Gecko/20101026 Firefox/3.6.12

When right-click -> View Page Source is used to display the HTML contents of a Web page, the resulting window has <link> and <script> tags conveniently displayed with links to their targets. However, these links are useless with a web page that contains <head><base href="some_directory/">, because the convenient links don't take into account the <base> tag.

Either the links themselves should contain the base reference, or Firefox should recognize that the displayed page has a <base> tag and act accordingly.

Reproducible: Always

Steps to Reproduce:
1. Right-click on any page that has a <base href=> tag/attribute and also contains <script> and/or <link> tags within the <head> tag, then select "View Page Source

2. Note the <base> tag and note the underlined links for <link> or <script> tag targets.

3. Click on the underlined link. The intended page is not displayed.
Actual Results:  
The intended page is not displayed, because the <base> tag is not honored when following the link.

Expected Results:  
The <base> tag should be used when following the link.
Component: Page Info → View Source
Product: Firefox → Toolkit
QA Contact: page.info → view.source
Version: unspecified → 1.9.2 Branch
I can confirm that.
See the test case attached at http://code.google.com/p/fbug/issues/detail?id=4886 comment 4. The page source dialog doesn't consider the base path being set to "css/".
Confirmed in Firefox 11 beta 3 on Mac, too (has been working fine in previous versions < 11)
I can echo ^ david verbatim here. Confirmed on latest Firefox 11 Beta on Mac 10.7.3 as well. Noticed that this recently stopped working on this build, though has previously not be an issue.
Confirming based on comments and STR in bug 736103
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 1.9.2 Branch → Trunk
Component: View Source → HTML: Parser
Product: Toolkit → Core
Whiteboard: [mentor=hsivonen][lang=C++]
(In reply to Wesley Chalmers from comment #11)
> This looks like an interesting way to familiarize myself with w3c specs and
> some of FF's features such as the Source viewer.

More the latter than the former.
Assignee: nobody → wwchalme
I assume the patch for this would be relatively easy. If so, you should add a keyword that it's a good starting issue for coding newbies.

Sebastian
Assignee: wwchalme → nobody
For anyone else who wants to tackle this, here are some interesting signposts to help you get started:
nsHtml5TreeOpExecutor::SetSpeculationBase, nsHtml5TreeOpExecutor::GetViewSourceBaseURI, eTreeOpAddViewSourceHref
Mentor: hsivonen
Whiteboard: [mentor=hsivonen][lang=C++] → [lang=C++]
Whiteboard: [lang=C++] → [good first bug][lang=C++]
Hi!

I want to work on this bug.
I have read the beginner's documentation and successfully built firefox nightly.
I reproduced the bug and have a good idea of it. What I am not able to understand is it's position in the source code.

Could u please guide me through the source code and where this fits in?
Hi Amanjot - I'm pretty sure that you'll want to be looking in parser/html/. You'll find methods like nsHtml5TreeOpExecutor::SetSpeculationBase in nsHtml5TreeOpExecutor.cpp, and mSpeculationBaseURI and mViewSourceBaseURI look interesting to me. Also, the eTreeOpAddViewSourceHref case in nsHtml5TreeOperation.cpp looks quite relevant to me.
hello, is somebody working on this bug because i would like to work on this bug, i know c++ this would be my first bug so i might need some extra guidance debugging it. thanks!
I haven't seen any further followup from Amanjot, so I think you're welcome to give it a try.
Attached patch bug617789.diff (obsolete) — Splinter Review
Attachment #8489809 - Flags: review?(hsivonen)
Comment on attachment 8489809 [details] [diff] [review]
bug617789.diff

Review of attachment 8489809 [details] [diff] [review]:
-----------------------------------------------------------------

This is unnecessarily complex, since this uses the speculative load machinery instead of adding a view source-specific check for <base>.

The better way to fix this would be in nsHtml5TreeBuilderCppSupplement.cpp in nsHtml5TreeBuilder::createElement locate "} else if (aNamespace == kNameSpaceID_XHTML && nsHtml5Atoms::html == aName) {" and change that to
} else if (aNamespace == kNameSpaceID_XHTML) {
  if (nsHtml5Atoms::html == aName) {
    // existing block
  } else if (nsHtml5Atoms::base == aName && mViewSource) {
    // new code to call something on mViewSource
  }
}
Attachment #8489809 - Flags: review?(hsivonen) → review-
In nsHtml5Highlighter, please, add a method called AddBase() that dispatches a new tree op  eTreeOpAddViewSourceBase similarly to how nsHtml5Highlighter::AddViewSourceHref works. The processing of the tree op should set mViewSourceBaseURI on nsHtml5TreeOpExecutor (new setter needed).

Then call AddBase() in the manner described in comment 25 on the bug.
Attached patch bug617789.diff (obsolete) — Splinter Review
Attachment #8489809 - Attachment is obsolete: true
Attachment #8496481 - Flags: review?(hsivonen)
Attached patch bug617789.diff (obsolete) — Splinter Review
Attachment #8496481 - Attachment is obsolete: true
Attachment #8496481 - Flags: review?(hsivonen)
Attachment #8496512 - Flags: review?(hsivonen)
Comment on attachment 8496512 [details] [diff] [review]
bug617789.diff

Review of attachment 8496512 [details] [diff] [review]:
-----------------------------------------------------------------

This patch needs various trivial fixes (see below), hence marking r- pending the fixes, but overall it looks like the right fix. Thanks.

::: parser/html/nsHtml5Highlighter.cpp
@@ +732,5 @@
>  void
> +nsHtml5Highlighter::AddBase(const nsString& aValue)
> +{
> +  if(BaseVisited)
> +    return;

Please use braces around the body of the |if| even if it's just one statement.

@@ +733,5 @@
> +nsHtml5Highlighter::AddBase(const nsString& aValue)
> +{
> +  if(BaseVisited)
> +    return;
> +  BaseVisited=1;

Please use true instead of 1 and add spaces on both sides of =.

@@ +741,5 @@
> +
> +  mOpQueue.AppendElement()->Init(eTreeOpAddViewSourceBase,
> +                                 bufferCopy,
> +                                 aValue.Length(),
> +                                 CurrentNode());

It seems to me that it isn't necessary to pass CurrentNode() as the last argument here. Please omit it.

::: parser/html/nsHtml5Highlighter.h
@@ +409,5 @@
> +    
> +    /**
> +     * Whether base is already visited once.
> +     */
> +     bool BaseVisited=0;

Please call the variable mSeenBase. Also, please omit "=0" here and initialize the field to false in the initializer list of nsHtml5Highlighter::nsHtml5Highlighter.

::: parser/html/nsHtml5TreeBuilderCppSupplement.h
@@ +229,5 @@
>             aAttributes->contains(nsHtml5AttributeName::ATTR_DEFER));
>        }
> +    } else if (aNamespace == kNameSpaceID_XHTML) {
> +      if (nsHtml5Atoms::html == aName)
> +      {

Please move the brace to the previous line.

::: parser/html/nsHtml5TreeOpExecutor.cpp
@@ +920,5 @@
>  void
> +nsHtml5TreeOpExecutor::AddBase(const nsAString& aURL)
> +{
> +  const nsCString& charset = mDocument->GetDocumentCharacterSet();
> +  DebugOnly<nsresult> rv = NS_NewURI(getter_AddRefs(mViewSourceBaseURI), aURL,

NS_NewURI can actually return a failure, e.g. if the URL fails to parse. Please change DebugOnly<nsresult> rv to nsresult rv and then add below:
if (NS_FAILED(rv)) {
  return;
}

@@ +921,5 @@
> +nsHtml5TreeOpExecutor::AddBase(const nsAString& aURL)
> +{
> +  const nsCString& charset = mDocument->GetDocumentCharacterSet();
> +  DebugOnly<nsresult> rv = NS_NewURI(getter_AddRefs(mViewSourceBaseURI), aURL,
> +                                     charset.get(), mViewSourceBaseURI);

Instead of using the potentially unintialized mViewSourceBaseURI here, please instead call GetViewSourceBaseURI() to make a relative <base href> resolve properly.

@@ +924,5 @@
> +  DebugOnly<nsresult> rv = NS_NewURI(getter_AddRefs(mViewSourceBaseURI), aURL,
> +                                     charset.get(), mViewSourceBaseURI);
> +  nsAutoCString Cspec;
> +  mViewSourceBaseURI->GetSpec(Cspec);
> +  printf("%s\n",Cspec.get());

Please omit these three lines.

::: parser/html/nsHtml5TreeOperation.cpp
@@ +112,5 @@
>        nsMemory::Free(mOne.unicharPtr);
>        break;
> +    case eTreeOpAddViewSourceBase:
> +      delete[] mTwo.unicharPtr;
> +      break;

Instead of adding all three lines, please instead just add "case eTreeOpAddViewSourceBase:" below case "eTreeOpAddViewSourceHref:" earlier in this "switch".

@@ +963,5 @@
>        return rv;
>      }
> +    case eTreeOpAddViewSourceBase: {
> +      char16_t* buffer = mTwo.unicharPtr;
> +      nsString baseurl(buffer);

Instead of the above two lines, please use
      char16_t* buffer = mTwo.unicharPtr;
      int32_t length = mFour.integer;
      nsDependentString baseUrl(buffer, length);

::: parser/html/nsHtml5TreeOperation.h
@@ +61,5 @@
>    eTreeOpAddLineNumberId,
>    eTreeOpAddErrorAtom,
>    eTreeOpAddErrorTwoAtoms,
> +  eTreeOpStartLayout,
> +  eTreeOpAddViewSourceBase

Plese add the new eTreeOpAddViewSourceBase before  eTreeOpStartLayout to group the view source-related operations together.
Attachment #8496512 - Flags: review?(hsivonen) → review-
Attached patch bug617789.diff (obsolete) — Splinter Review
hsivonen: thanks for taking time and pointing out my mistakes in the previous patch now i have a better understanding of coding practices here. hope this patch fixes everything.
Attachment #8496512 - Attachment is obsolete: true
Attachment #8497323 - Flags: review?(hsivonen)
Comment on attachment 8497323 [details] [diff] [review]
bug617789.diff

Review of attachment 8497323 [details] [diff] [review]:
-----------------------------------------------------------------

Looks very good. Thanks.

::: parser/html/nsHtml5TreeOpExecutor.cpp
@@ +923,5 @@
> +  const nsCString& charset = mDocument->GetDocumentCharacterSet();
> +  nsresult rv = NS_NewURI(getter_AddRefs(mViewSourceBaseURI), aURL,
> +                                     charset.get(), GetViewSourceBaseURI());
> +  if (NS_FAILED(rv)) {
> +    return;

Sorry, my mistake in my previous comments: instead of "return", this should say "mViewSourceBaseURI = nullptr". r+ with that changed.
Attachment #8497323 - Flags: review?(hsivonen) → review+
Attached patch bug617789.diff (obsolete) — Splinter Review
hsivonen: i have fixed the patch as per your last comment. also what do i need to do next. thanks!
Attachment #8497323 - Attachment is obsolete: true
Attachment #8497370 - Flags: review+
Attached patch RebasedSplinter Review
Usually, the next step is asking someone with tryserver to push the patch to try. I went ahead and pushed it to try:
https://tbpl.mozilla.org/?tree=Try&rev=7c6bc8ae25fb

Attaching a trivially rebased version of the patch with the commit message adjusted (spaces around the bug number and r=hsivonen in the end).

Once you see that the try run is OK (no new failures caused by the patch), the next step is adding the "checkin-needed" keyword to the Keywords field of the bug.
Attachment #8497370 - Attachment is obsolete: true
Attachment #8497386 - Flags: review+
(In reply to Henri Sivonen (:hsivonen) from comment #33)
> Created attachment 8497386 [details] [diff] [review]
> Rebased
> 
> Usually, the next step is asking someone with tryserver to push the patch to
> try. I went ahead and pushed it to try:
> https://tbpl.mozilla.org/?tree=Try&rev=7c6bc8ae25fb
> 
> Attaching a trivially rebased version of the patch with the commit message
> adjusted (spaces around the bug number and r=hsivonen in the end).
> 
> Once you see that the try run is OK (no new failures caused by the patch),
> the next step is adding the "checkin-needed" keyword to the Keywords field
> of the bug.

has the patch passed the tests? i can see some red marks on try run, if these are failures what should i do to fix these?
(In reply to diwas joshi [:dij] from comment #34)
> has the patch passed the tests? i can see some red marks on try run, if
> these are failures what should i do to fix these?

The failures look unrelated to this patch. It's OK to request landing.
(In reply to Henri Sivonen (:hsivonen) from comment #35)
> (In reply to diwas joshi [:dij] from comment #34)
> > has the patch passed the tests? i can see some red marks on try run, if
> > these are failures what should i do to fix these?
> 
> The failures look unrelated to this patch. It's OK to request landing.

also the "assigned to" field of this bug says to nobody can this be assigned to me now?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7ec922b5772f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Finally works fine for me. Thank you very much and congratulations for your first bug fix, Diwas!

Sebastian
(In reply to Sebastian Zartner from comment #39)
> Finally works fine for me. Thank you very much and congratulations for your
> first bug fix, Diwas!
> 
> Sebastian

thanks a lot!! :) it really was possible only because of all the help from henri sivonen.
This issue is observed again on Firefox 45.0.2, Please take a look.
It's still working fine for me using Firefox 46.0 on Windows 7. Please file a new bug and add a test case to it.

Sebastian
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.