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.

VERIFIED FIXED in mozilla34

Status

()

Toolkit
View Source
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: Fanolian, Assigned: hharchani, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla34
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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]
(Reporter)

Updated

3 years ago
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@gmail.com
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]
(Assignee)

Comment 2

3 years ago
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
(Assignee)

Comment 4

3 years ago
Created attachment 8472983 [details] [diff] [review]
bug-1051221.patch

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+
(Assignee)

Comment 7

3 years ago
Created attachment 8473677 [details] [diff] [review]
bug-1051221.patch

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+
(Assignee)

Comment 9

3 years ago
Created attachment 8475548 [details] [diff] [review]
bug-1051221.patch
Attachment #8473677 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #8475548 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/459e1a537e75
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/459e1a537e75
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [good first verify]
Flags: qe-verify+
Created attachment 8479879 [details]
Result of Save in latest nightly after patching

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.