966744 - View Selection Source completely broken

Status: RESOLVED WORKSFORME

(Core :: DOM: Core & HTML, defect)

Description

[Alice0775 White]

Steps To Reproduce (for example)
1. Open https://developer.mozilla.org/en-US/Add-ons/Code_snippets?redirectlocale=en-US&redirectslug=Code_snippets
2. Select All (Ctrl+A)
3. Right click and Choose "View Selection Source"

Actual Results:
It shows few lines only

Expected Results:
It should be 663 lines

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/6fc89dd13c81
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20131219050507
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5de427ba924e
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20131219061007
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6fc89dd13c81&tochange=5de427ba924e

Regressed by: Bug 943519

Comment 1

[Benjamin Kerensa [:bkerensa]]

Alessio, can you have a look at this since it is your patch? Thanks

Comment 2

[Alessio Placitelli [:Dexter]]

(In reply to Benjamin Kerensa [:bkerensa] from comment #1)
> Alessio, can you have a look at this since it is your patch? Thanks

Sure, I'll give it a look ASAP!

Comment 3

[(no longer active)]

We should get some tests for this stuff at some point.  :(

Comment 4

[Henri Sivonen (:hsivonen)]

The regressing  bug  added rv  propagation where there was none before. I bet that View Selection Source relies on failure rv  not being propagated somewhere.

Comment 5

[Henri Sivonen (:hsivonen)]

Same root cause as bug 964239? Let's see if that one fixed this, too.

Comment 6

[Alessio Placitelli [:Dexter]]

This is what happens when running a debug build:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x804B000A: file /Mozilla-source-local/Mozilla-central/parser/html/nsHtml5TreeOperation.cpp, line 756

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: bug966744.patch

By following the precious advices of Benjamin Smedberg, I was able to pinpoint the issue and deliver this fix. I'm working on a mochitest right now to automatically test for this bug.

Comment 8

[Alessio Placitelli [:Dexter]]

Just uploaded the patch to the tryserv to be sure:

https://tbpl.mozilla.org/?tree=Try&rev=759ff40143c8

Comment 9

[Henri Sivonen (:hsivonen)]

The patches for the other bug mentioned in comment 5 landed:
https://hg.mozilla.org/mozilla-central/rev/028dbae2c819

This bug is now fixed, right?

Comment 10

[Henri Sivonen (:hsivonen)]

We don't need this for branch landing or anything, right?

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Henri, AFAIK, the patches from the other bug has not been uplifted to aurora (and the uplift request should probably been done in the other bug)

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 8372779 [details] [diff] [review]
bug966744.patch

r+ for Aurora only (if approved, requesting approval now.)

> [Approval Request Comment]
> Bug caused by (feature/regressing bug #):

Bug 943519.
 
> User impact if declined: 

The View Selection Source is broken if the any URL attributes (e.g. href on <a>!) are involved.

> Testing completed (on m-c, etc.): 

An equivalent patch has landed on m-c  as part of another bug fix.

> Risk to taking this patch (and alternatives if risky): 

Extremely low-risk.

> String or IDL/UUID changes made by this patch:

None.

Comment 13

[Henri Sivonen (:hsivonen)]

checkin-needed to Aurora.

Comment 14

[Sylvestre Ledru [:Sylvestre]]

In the case of uplift, checkin-needed is not needed. Sheriffs go on the list of approval-mozilla-aurora+ few times a day.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

For future reference, it's generally best to leave the aurora approval requests in the original bug rather than moving the patch over - that's just confusing.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/746c1ac50aa0

Comment 17

[Boris Zbarsky [:bzbarsky]]

Did bug 964239 actually fix this?  It seems weird to me to treat that NewURI call as fatal for parsing purposes; it can fail for reasons other than lack of a base URI, no?

Comment 18

[Alessio Placitelli [:Dexter]]

(In reply to Boris Zbarsky [:bz] from comment #17)
> Did bug 964239 actually fix this?  It seems weird to me to treat that NewURI
> call as fatal for parsing purposes; it can fail for reasons other than lack
> of a base URI, no?

If it can fail for other reasons, then it's probably more correct to treat it differently. But, to be honest, I'm not quite an expert so it's better if :hsivonen answers your questions!

Comment 19

[Henri Sivonen (:hsivonen)]

Bug 998356 comment 1 seems to have the right reason.

Comment 20

[Alessio Placitelli [:Dexter]]

Is this bug still valid?

Comment 21

[Ciprian Muresan [:cmuresan], Ecosystem QA]

Hi,

I haven't managed to reproduce the issue on the latest Firefox release(45.0.2, Build ID 20160407164938) or the latest Nightly(48.0a1, Build ID 20160415030231) and can confirm this was an issue on the Nightly 29 (20131219061007). 
Considering this I will mark this issue as Resolved-WORKSFORME. If anyone can still reproduce it, feel free to reopen the issue and provide more information.

Thanks,
Cipri