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)
Tracking
()
VERIFIED
FIXED
mozilla35
People
(Reporter: pscott, Assigned: dij, Mentored)
References
Details
(Whiteboard: [good first bug][lang=C++])
Attachments
(1 file, 5 obsolete files)
9.22 KB,
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•14 years ago
|
Component: Page Info → View Source
Product: Firefox → Toolkit
QA Contact: page.info → view.source
Version: unspecified → 1.9.2 Branch
Comment 1•13 years ago
|
||
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.
Comment 5•12 years ago
|
||
Confirming based on comments and STR in bug 736103
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 1.9.2 Branch → Trunk
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Updated•11 years ago
|
Component: View Source → HTML: Parser
Product: Toolkit → Core
Whiteboard: [mentor=hsivonen][lang=C++]
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment 12•11 years ago
|
||
(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
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment 15•10 years ago
|
||
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
Comment hidden (obsolete) |
Comment hidden (advocacy) |
Comment hidden (obsolete) |
Updated•10 years ago
|
Assignee: wwchalme → nobody
Comment 19•10 years ago
|
||
For anyone else who wants to tackle this, here are some interesting signposts to help you get started: nsHtml5TreeOpExecutor::SetSpeculationBase, nsHtml5TreeOpExecutor::GetViewSourceBaseURI, eTreeOpAddViewSourceHref
Updated•10 years ago
|
Mentor: hsivonen
Whiteboard: [mentor=hsivonen][lang=C++] → [lang=C++]
Updated•10 years ago
|
Whiteboard: [lang=C++] → [good first bug][lang=C++]
Comment 20•10 years ago
|
||
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?
Comment 21•10 years ago
|
||
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.
Assignee | ||
Comment 22•10 years ago
|
||
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!
Comment 23•10 years ago
|
||
I haven't seen any further followup from Amanjot, so I think you're welcome to give it a try.
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8489809 -
Flags: review?(hsivonen)
Comment 25•10 years ago
|
||
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-
Comment 26•10 years ago
|
||
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.
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8489809 -
Attachment is obsolete: true
Attachment #8496481 -
Flags: review?(hsivonen)
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8496481 -
Attachment is obsolete: true
Attachment #8496481 -
Flags: review?(hsivonen)
Attachment #8496512 -
Flags: review?(hsivonen)
Comment 29•10 years ago
|
||
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-
Assignee | ||
Comment 30•10 years ago
|
||
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 31•10 years ago
|
||
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+
Assignee | ||
Comment 32•10 years ago
|
||
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+
Comment 33•10 years ago
|
||
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+
Assignee | ||
Comment 34•10 years ago
|
||
(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?
Comment 35•10 years ago
|
||
(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.
Assignee | ||
Comment 36•10 years ago
|
||
(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
Comment 37•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ec922b5772f
Assignee: nobody → dj.dij123
Updated•10 years ago
|
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7ec922b5772f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 39•10 years ago
|
||
Finally works fine for me. Thank you very much and congratulations for your first bug fix, Diwas! Sebastian
Assignee | ||
Comment 40•10 years ago
|
||
(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.
Comment 41•8 years ago
|
||
This issue is observed again on Firefox 45.0.2, Please take a look.
Comment 42•8 years ago
|
||
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.
Description
•