Closed Bug 1051221 Opened 10 years ago Closed 10 years ago

Saving MathML source fails - view source shouldn't blindly substring its href because MathML source doesn't start with view-source: protocol but with data: instead.

Categories

(Toolkit :: View Source, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla34

People

(Reporter: Fanolian+BMO, Assigned: hharchani, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(2 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 (Beta/Release)
Build ID: 20140808030201

Steps to reproduce:

1. Visit any MathML sources, e.g. https://developer.mozilla.org/en-US/docs/Mozilla/MathML_Project/MathML_Torture_Test
2. Right click on a MathML source and choose "View MathML Source".
3. Try to save the source from File > Save page as…


Actual results:

No files is created. In browser console it shows 2 errors after clicking save:
22:01:29.168 aSource is null DownloadLegacy.js:232
22:01:29.168 [Exception... "[JavaScript Error: "aSource is null" {file: "resource://gre/components/DownloadLegacy.js" line: 232}]'[JavaScript Error: "aSource is null" {file: "resource://gre/components/DownloadLegacy.js" line: 232}]' when calling method: [nsITransfer::init]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 385"  data: yes] Promise-backend.js:869



Expected results:

A file should be created.


Note:
On the same page, I can save the page source. Only MathML source is not working.
This is not a recent regression. I can reproduce it as far back as 2012-01-01 build with a different error message in error console:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004003 (NS_ERROR_INVALID_POINTER) [nsITransfer.init]"  nsresult: "0x80004003 (NS_ERROR_INVALID_POINTER)"  location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 389"  data: no]
Component: Untriaged → File Handling
Component: File Handling → View Source
Product: Firefox → Toolkit
Summary: Unable to save MathML sources → saving MathML source fails with nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"
So the issue is that this code:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/viewsource/content/viewSource.js?rev=69d61e42d5df&mark=326-328#323

is dumb and just substrings the content href, expecting it to be of the form "view-source:whatever", leaving the "whatever" bit.

That would be fine except that for MathML view source, the content href is of the form:

data:text/html,...

I don't really know why.

In any case, I expect this could be fixed by making the above-linked bit of code slightly smarter and just, say, regex-replace /^view-source:/i with "" instead.

Mentoring this as a good first bug.
Mentor: gijskruitbosch+bugs
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 8.1 → All
Hardware: x86_64 → All
Summary: saving MathML source fails with nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)" → Saving MathML source fails - view source shouldn't blindly substring its href because MathML source doesn't start with view-source: protocol but with data: instead.
Whiteboard: [good first bug][lang=js]
I want to work on this Bug, assign this to me.
(In reply to hharchani from comment #2)
> I want to work on this Bug, assign this to me.

Sounds good! Let me know if you need more help beyond comment #1. :-)
Assignee: nobody → hharchani
Status: NEW → ASSIGNED
Attached patch bug-1051221.patch (obsolete) — Splinter Review
Successfully build and run. Now file is being saved.
Attachment #8472983 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8472983 [details] [diff] [review]
bug-1051221.patch

I'm sorry, I'm technically not a toolkit peer, so I'm redirecting the review to Matt. :-)
Attachment #8472983 - Flags: review?(gijskruitbosch+bugs) → review?(MattN+bmo)
Comment on attachment 8472983 [details] [diff] [review]
bug-1051221.patch

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

The alternative solution would be to make MathML also use |view-source:| but it wasn't immediately obvious to me how to do that at quick glance and this is definitely an improvement.

Thanks

::: toolkit/components/viewsource/content/viewSource.js
@@ +322,5 @@
>  
>  // Strips the |view-source:| for internalSave()
>  function ViewSourceSavePage()
>  {
> +  internalSave(window.content.location.href.replace(/^view-source:/i,""),

Please include a space after the comma to separate the arguments.
Attachment #8472983 - Flags: review?(MattN+bmo) → review+
Attached patch bug-1051221.patch (obsolete) — Splinter Review
Added a space to separate parameters.
Attachment #8472983 - Attachment is obsolete: true
Attachment #8473677 - Flags: review?(MattN+bmo)
Comment on attachment 8473677 [details] [diff] [review]
bug-1051221.patch

Hello,

Since I gave r+ with only simple changes, you don't need to re-request review. You should update the commit message before requesting checkin-needed though. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Committing_Rules_and_Responsibilities#Checkin_comment

Thanks
Attachment #8473677 - Flags: review?(MattN+bmo) → review+
Attachment #8473677 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #8475548 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/459e1a537e75
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [good first verify]
Flags: qe-verify+
Hi, I've begun to work on this bug for QA's verification bugday.
I've noticed that while the problem seems to have been fixed, after reviewing older builds of Nightly and the newest one, there's an inconsistency of behavior.
When I try to save source of an HTML file, or otherwise when I make a partial selection and try to save the selected source, I always get a file with the actual code inside, and thus, when opened in Firefox, I get a webpage, or in the other case I get the element.
For MathMl however, the Save command will actually save the HTML *view* of the MathML view-source window. This means that the file produced, when opened, will not display the actual equation, but the source in a stylized HTML manner.
Is this the intended behavior?
QA Whiteboard: [good first verify] → [good first verify] [bugday-20140827]
I'm sorry the Comment #12 did not produce the need-info even though I had selected the option (a bug in the attachment form?).
Mentor, can you please answer the question on that comment? Thanks :)
(In reply to Ricardo Maçãs [:Ricmacas] from comment #13)
> I'm sorry the Comment #12 did not produce the need-info even though I had
> selected the option (a bug in the attachment form?).
> Mentor, can you please answer the question on that comment? Thanks :)

Fascinating! This is not ideal, no, but I think we should figure out the right behaviour here in a followup, as the current behaviour is strictly better than the previous. :-)

Can you file a followup bug?
(In reply to :Gijs Kruitbosch from comment #14)
> (In reply to Ricardo Maçãs [:Ricmacas] from comment #13)
> > I'm sorry the Comment #12 did not produce the need-info even though I had
> > selected the option (a bug in the attachment form?).
> > Mentor, can you please answer the question on that comment? Thanks :)
> 
> Fascinating! This is not ideal, no, but I think we should figure out the
> right behaviour here in a followup, as the current behaviour is strictly
> better than the previous. :-)
> 
> Can you file a followup bug?

Of course, thanks for answering, it seems all my needinfos are going to /dev/null and I was afraid I was spamming you with bugmail already.
I'll file a followup bug, should we VERIFY this bug then?
(I'll try needinfo with "other" this time).
Flags: needinfo?(gijskruitbosch+bugs)
Bug 1059297 filed as requested.
Yup, I think we can mark this verified. Thanks!
Blocks: 1059297
Status: RESOLVED → VERIFIED
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.