Closed Bug 966744 Opened 7 years ago Closed 5 years ago

View Selection Source completely broken

Categories

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

29 Branch
x86_64
All
defect
Not set
major

Tracking

()

RESOLVED WORKSFORME
mozilla30
Tracking Status
firefox28 --- unaffected
firefox29 + fixed
firefox30 --- fixed

People

(Reporter: alice0775, Unassigned)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
OS: Windows 7 → All
Alessio, can you have a look at this since it is your patch? Thanks
Flags: needinfo?(alessio.placitelli)
(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!
Flags: needinfo?(alessio.placitelli)
Summary: Veiw Selection Source completely broken → View Selection Source completely broken
We should get some tests for this stuff at some point.  :(
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.
Same root cause as bug 964239? Let's see if that one fixed this, too.
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
Attached patch bug966744.patchSplinter Review
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.
Assignee: nobody → alessio.placitelli
Attachment #8372779 - Flags: review?(hsivonen)
Just uploaded the patch to the tryserv to be sure:

https://tbpl.mozilla.org/?tree=Try&rev=759ff40143c8
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?
We don't need this for branch landing or anything, right?
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 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.
Attachment #8372779 - Flags: review?(hsivonen)
Attachment #8372779 - Flags: review+
Attachment #8372779 - Flags: approval-mozilla-aurora?
Attachment #8372779 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
checkin-needed to Aurora.
In the case of uplift, checkin-needed is not needed. Sheriffs go on the list of approval-mozilla-aurora+ few times a day.
Keywords: checkin-needed
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.
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?
Flags: needinfo?(alessio.placitelli)
(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!
Flags: needinfo?(alessio.placitelli) → needinfo?(hsivonen)
Bug 998356 comment 1 seems to have the right reason.
Flags: needinfo?(hsivonen)
Is this bug still valid?
Assignee: alessio.placitelli → nobody
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
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.